Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

refactor(JP): remove arrow usage #6572

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

refactor(JP): remove arrow usage #6572

wants to merge 1 commit into from
+29 −25

Conversation

shuuji3
Copy link
Contributor

@shuuji3 shuuji3 commented Mar 22, 2024 •
edited

Issue

#6135

Description

It looks like the price exchange data source was updated and failed to run it by poetry run ./test_parser.py JP-KY price so I couldn't test the change for price task.

Execution result on master and this branch:

poetry run ./test_parser.py JP-KY price
Traceback (most recent call last):
  File "/home/shuuji3/dev/electricitymap-contrib/./test_parser.py", line 137, in <module>
    print(test_parser())
  File "/home/shuuji3/dev/electricitymap-contrib/venv/lib/python3.10/site-packages/click/core.py", line 1134, in __call__
    return self.main(*args, **kwargs)
  File "/home/shuuji3/dev/electricitymap-contrib/venv/lib/python3.10/site-packages/click/core.py", line 1059, in main
    rv = self.invoke(ctx)
  File "/home/shuuji3/dev/electricitymap-contrib/venv/lib/python3.10/site-packages/click/core.py", line 1401, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/home/shuuji3/dev/electricitymap-contrib/venv/lib/python3.10/site-packages/click/core.py", line 767, in invoke
    return __callback(*args, **kwargs)
  File "/home/shuuji3/dev/electricitymap-contrib/./test_parser.py", line 66, in test_parser
    res = parser(
  File "/home/shuuji3/dev/electricitymap-contrib/parsers/lib/config.py", line 20, in wrapped_f
    result = f(*args, **kwargs)
  File "/home/shuuji3/dev/electricitymap-contrib/parsers/JP.py", line 284, in fetch_price
    df = pd.read_csv(url, encoding="shift-jis")
  File "/home/shuuji3/dev/electricitymap-contrib/venv/lib/python3.10/site-packages/pandas/util/_decorators.py", line 211, in wrapper
    return func(*args, **kwargs)
  File "/home/shuuji3/dev/electricitymap-contrib/venv/lib/python3.10/site-packages/pandas/util/_decorators.py", line 331, in wrapper
    return func(*args, **kwargs)
  File "/home/shuuji3/dev/electricitymap-contrib/venv/lib/python3.10/site-packages/pandas/io/parsers/readers.py", line 950, in read_csv
    return _read(filepath_or_buffer, kwds)
  File "/home/shuuji3/dev/electricitymap-contrib/venv/lib/python3.10/site-packages/pandas/io/parsers/readers.py", line 605, in _read
    parser = TextFileReader(filepath_or_buffer, **kwds)
  File "/home/shuuji3/dev/electricitymap-contrib/venv/lib/python3.10/site-packages/pandas/io/parsers/readers.py", line 1442, in __init__
    self._engine = self._make_engine(f, self.engine)
  File "/home/shuuji3/dev/electricitymap-contrib/venv/lib/python3.10/site-packages/pandas/io/parsers/readers.py", line 1753, in _make_engine
    return mapping[engine](f, **self.options)
  File "/home/shuuji3/dev/electricitymap-contrib/venv/lib/python3.10/site-packages/pandas/io/parsers/c_parser_wrapper.py", line 79, in __init__
    self._reader = parsers.TextReader(src, **kwds)
  File "pandas/_libs/parsers.pyx", line 547, in pandas._libs.parsers.TextReader.__cinit__
  File "pandas/_libs/parsers.pyx", line 636, in pandas._libs.parsers.TextReader._get_header
  File "pandas/_libs/parsers.pyx", line 852, in pandas._libs.parsers.TextReader._tokenize_rows
  File "pandas/_libs/parsers.pyx", line 1965, in pandas._libs.parsers.raise_parser_error
UnicodeDecodeError: 'shift_jis' codec can't decode byte 0x80 in position 140: illegal multibyte sequence

Preview

N/A

Double check

  • I have tested my parser changes locally with poetry run test_parser "zone_key"
  • I have run pnpx prettier@2 --write . and poetry run format in the top level directory to format my changes.

@github-actions github-actions bot added parser python Pull requests that update Python code labels Mar 22, 2024
@shuuji3 shuuji3 marked this pull request as ready for review March 23, 2024 11:18
shuuji3
shuuji3 commented Mar 23, 2024
View reviewed changes
parsers/JP.py
@@ -210,9 +215,14 @@ def fetch_consumption_forecast(
# Currently past dates not implemented for areas with no date in their demand csv files
if target_datetime and zone_key == "JP-HKD":
raise NotImplementedError("Past dates not yet implemented for selected region")
datestamp = arrow.get(target_datetime).to("Asia/Tokyo").strftime("%Y%m%d")

if target_datetime is None:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When target_datetime == None, arrow.get(target_datetime) returns the current time.

VIKTORVAV99
VIKTORVAV99 requested changes Mar 25, 2024
View reviewed changes
Copy link
Member

@VIKTORVAV99 VIKTORVAV99 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately running poetry run test_parser JP-CB fails with:

Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/Users/viktor/Repos/electricitymaps-contrib/.venv/lib/python3.10/site-packages/click/core.py", line 1134, in __call__
    return self.main(*args, **kwargs)
  File "/Users/viktor/Repos/electricitymaps-contrib/.venv/lib/python3.10/site-packages/click/core.py", line 1059, in main
    rv = self.invoke(ctx)
  File "/Users/viktor/Repos/electricitymaps-contrib/.venv/lib/python3.10/site-packages/click/core.py", line 1401, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/Users/viktor/Repos/electricitymaps-contrib/.venv/lib/python3.10/site-packages/click/core.py", line 767, in invoke
    return __callback(*args, **kwargs)
  File "/Users/viktor/Repos/electricitymaps-contrib/test_parser.py", line 66, in test_parser
    res = parser(
  File "/Users/viktor/Repos/electricitymaps-contrib/parsers/lib/config.py", line 20, in wrapped_f
    result = f(*args, **kwargs)
  File "/Users/viktor/Repos/electricitymaps-contrib/parsers/JP.py", line 69, in fetch_production
    df = fetch_production_df(zone_key, session, target_datetime)
  File "/Users/viktor/Repos/electricitymaps-contrib/parsers/JP.py", line 135, in fetch_production_df
    df = pd.merge(df, df2, how="inner", on="datetime")
  File "/Users/viktor/Repos/electricitymaps-contrib/.venv/lib/python3.10/site-packages/pandas/core/reshape/merge.py", line 110, in merge
    op = _MergeOperation(
  File "/Users/viktor/Repos/electricitymaps-contrib/.venv/lib/python3.10/site-packages/pandas/core/reshape/merge.py", line 707, in __init__
    self._maybe_coerce_merge_keys()
  File "/Users/viktor/Repos/electricitymaps-contrib/.venv/lib/python3.10/site-packages/pandas/core/reshape/merge.py", line 1346, in _maybe_coerce_merge_keys
    raise ValueError(msg)
ValueError: You are trying to merge on object and datetime64[ns, tzfile('/usr/share/zoneinfo/Asia/Tokyo')] columns. If you wish to proceed you should use pd.concat

This needs to be fixed before this can be merged.

parsers/JP.py
Comment on lines +345 to +346
date = row["Date"]
time = row["Time"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it not be easier to parse these directly using the date and time libraries and then just combine them instead of using string manipulation?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks only datetime library has strptime() but there is no string parser in date and time standard libraries (ref. https://docs.python.org/3.10/library/datetime.html#strftime-strptime-behavior). I wonder if there's another method other than datetime.strptime().

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

datetime.time has their own parsing functions, same with date.

https://docs.python.org/3.10/library/time.html#time.strptime

You could also use the date time library directly on each of them and just call .date() or .time() to get the individual parts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@VIKTORVAV99 VIKTORVAV99 VIKTORVAV99 requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
parser python Pull requests that update Python code
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@shuuji3 @VIKTORVAV99