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.

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

refactor(us-mida-pjm): upgrade US-MIDA-PJM parser with event classes #6635

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

amv213
Copy link
Contributor

@amv213 amv213 commented Apr 5, 2024

Issue

#6011

Description

This PR upgrades the US-MIDA-PJM parser to use event classes.

I have also fixed the .fetch_exchange() function, and expanded it to support all possible neighbouring exchanges. I also fixed a small bug that I found.

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 zone config Pull request or issue for zone configurations labels Apr 5, 2024
config/zones/US-MIDA-PJM.yaml Show resolved Hide resolved
parsers/US_PJM.py Show resolved Hide resolved
parsers/US_PJM.py Show resolved Hide resolved
parsers/US_PJM.py Show resolved Hide resolved
@amv213 amv213 marked this pull request as ready for review April 5, 2024 09:20
@amv213 amv213 requested a review from VIKTORVAV99 as a code owner April 5, 2024 09:20
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.

Will give another review as well since this is a big PR but here are some early comments.

parsers/US_PJM.py Show resolved Hide resolved
parsers/US_PJM.py Outdated Show resolved Hide resolved
parsers/US_PJM.py Show resolved Hide resolved
parsers/US_PJM.py Show resolved Hide resolved
Comment on lines 301 to +306
if target_datetime is not None:
raise NotImplementedError("This parser is not yet able to parse past dates")
raise ParserException(
PARSER,
"This parser is not yet able to parse historical data",
sorted_zone_keys,
)
Copy link
Member

Choose a reason for hiding this comment

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

This might be a blocker for the above though...

We generally prefer parsers that can fetch historical data (even if it's delayed) over realtime only parsers.

However in the future we might support using multiple parsers for the same data types then this would be really valuable.

Regardless I'll check with the team.

@amv213
Copy link
Contributor Author

amv213 commented Apr 8, 2024

Perfect - thank you so much @VIKTORVAV99! no rush on my side

@VIKTORVAV99 VIKTORVAV99 self-requested a review November 7, 2024 13:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
parser python Pull requests that update Python code zone config Pull request or issue for zone configurations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants