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

Upgrade IE parser to ParserFunctions #6042

Merged
merged 17 commits into from
Jan 14, 2024
Merged

Conversation

VIKTORVAV99
Copy link
Member

@VIKTORVAV99 VIKTORVAV99 commented Oct 21, 2023

Issue

The IE parser was not using ParserFunctions

Description

This PR upgrades the existing parser to use parser functions as well as adding a parser function for total generation.

Double check

  • I have tested my parser changes locally with poetry run test_parser "zone_key"
  • I have run pnpx prettier --write . and poetry run format to format my changes.

@github-actions github-actions bot added the zone config Pull request or issue for zone configurations label Nov 5, 2023
@github-actions github-actions bot added tests exchange config Pull request or issue for exchange configurations labels Nov 6, 2023
@github-actions github-actions bot added the python Pull requests that update Python code label Dec 23, 2023
productionPerModeForecast: IE.fetch_wind_forecasts
production: SMARTGRIDDASHBOARD.fetch_production
productionPerModeForecast: SMARTGRIDDASHBOARD.fetch_wind_forecasts
generationForecast: SMARTGRIDDASHBOARD.fetch_generation
Copy link
Member Author

Choose a reason for hiding this comment

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

Apparently we don't have a generation type for the parsers functions? Only generationForecast?

Copy link
Member Author

Choose a reason for hiding this comment

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

If it's easier to review I can rename this back to the original name and then just switch back before merging it.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds like a good idea :) Similarly, is there a file for the tests we previously used?

Copy link
Member Author

Choose a reason for hiding this comment

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

We did not have any tests before 😅

@VIKTORVAV99 VIKTORVAV99 marked this pull request as ready for review December 23, 2023 21:27
@VIKTORVAV99
Copy link
Member Author

/review

Copy link
Contributor

PR Analysis

  • 🎯 Main theme: Upgrading the IE parser to use ParserFunctions and adding a parser function for total generation.
  • 📝 PR summary: This PR focuses on upgrading the existing parser for Ireland (IE) to use parser functions. It also introduces a new parser function for total generation. The changes include modifications to the parser file, tests, and configuration files for the zones and exchanges.
  • 📌 Type of PR: Enhancement
  • 🧪 Relevant tests added: Yes
  • ⏱️ Estimated effort to review [1-5]: 4, because the PR includes a significant amount of new code and changes across multiple files. It requires a good understanding of the parser functions and the specific implementation for the Ireland parser.
  • 🔒 Security concerns: No

PR Feedback

  • 💡 General suggestions: The PR is well-structured and includes relevant tests. It would be beneficial to ensure that all edge cases and potential exceptions are handled properly. Also, it's important to verify that the new parser function for total generation is working as expected and providing accurate data.

  • 🤖 Code feedback:
    relevant fileparsers/SMARTGRIDDASHBOARD.py
    suggestion      Consider adding error handling for the case where the 'kind' parameter is not in the KINDS_AREA_MAPPING dictionary. This could prevent potential KeyErrors. [important]
    relevant line"+ \"area\": KINDS_AREA_MAPPING[kind],"

    relevant fileparsers/SMARTGRIDDASHBOARD.py
    suggestion      It might be beneficial to add some logging statements in the fetch_data function to provide more visibility into the data fetching process, especially in case of errors. [medium]
    relevant line"+def fetch_data("

    relevant fileparsers/SMARTGRIDDASHBOARD.py
    suggestion      Consider adding a check to ensure that the 'Value' key exists in the 'item' dictionary before accessing it. This could help avoid potential KeyErrors. [important]
    relevant line"+ consumption=item[\"Value\"],"

    relevant fileparsers/SMARTGRIDDASHBOARD.py
    suggestion      It would be good to add error handling for the case where the 'zone_key' parameter is not in the REGION_MAPPING or EXCHANGE_MAPPING dictionaries. This could prevent potential KeyErrors. [important]
    relevant line"+ \"region\": EXCHANGE_MAPPING[zone_key][\"key\"]"

How to use

Instructions

To invoke the PR-Agent, add a comment using one of the following commands:
/review: Request a review of your Pull Request.
/describe: Update the PR title and description based on the contents of the PR.
/improve [--extended]: Suggest code improvements. Extended mode provides a higher quality feedback.
/ask <QUESTION>: Ask a question about the PR.
/update_changelog: Update the changelog based on the PR's contents.
/add_docs: Generate docstring for new components introduced in the PR.
/generate_labels: Generate labels for the PR based on the PR's contents.
see the tools guide for more details.

To edit any configuration parameter from the configuration.toml, add --config_path=new_value.
For example: /review --pr_reviewer.extra_instructions="focus on the file: ..."
To list the possible configuration parameters, add a /config comment.

@VIKTORVAV99
Copy link
Member Author

@florianscheidl adding you as a reviewer for this as I saw you where working on ELE-3449, this PR will give us some additional data that could be used to guide the estimation models.

parsers/IE.py Outdated Show resolved Hide resolved
Copy link
Member

@florianscheidl florianscheidl left a comment

Choose a reason for hiding this comment

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

Overall looks very nice to me :)
I left some comments asking for clarification regarding fetch_generation and fetch_production, which confused me a bit.

@VIKTORVAV99
Copy link
Member Author

I'll rename things again and then merge this 🙂

@VIKTORVAV99 VIKTORVAV99 merged commit 78a2b61 into master Jan 14, 2024
19 checks passed
@VIKTORVAV99 VIKTORVAV99 deleted the vik/upgrade_IE_parser branch January 14, 2024 11:04
ghost pushed a commit that referenced this pull request Jan 24, 2024
* WIP upgrade parser functions

* upgrade production

* Add total generation

* add consumption test

* fix

* Add more tests

* Fix exception handling in SMARTGRIDDASHBOARD.py

* temp rename of parser file

* switch back in yaml files

* add missing dots

* format

* rename function and parser

---------

Co-authored-by: Florian Scheidl <36441589+florianscheidl@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
exchange config Pull request or issue for exchange configurations hacktoberfest-accepted parser python Pull requests that update Python code tests zone config Pull request or issue for zone configurations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants