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

Add CN-MN exchange and parser #6283

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

Conversation

silimotion
Copy link
Contributor

Description

This PR adds the CN->MN exchange. To calculate the exchange, I used the total imports data from the MN source, and the MN->RU-2 from the Russian source. This is based on the assumption that Mongolia only trades with this two countries. I think this is a reasonable assumption, given geography and Mongolian grid.

Preview

Screenshot_20231222_130854

Parser result:
[{'datetime': datetime.datetime(2023, 12, 22, 19, 0, tzinfo=zoneinfo.ZoneInfo(key='Asia/Ulaanbaatar')),
  'netFlow': 619.82134,
  'sortedZoneKeys': 'CN->MN',
  'source': 'https://ndc.energy.mn/',
  'sourceType': <EventSourceType.measured: 'measured'>}]
---------------------
took 4.39s
min returned datetime: 2023-12-22 11:00:00+00:00 UTC
max returned datetime: 2023-12-22 11:00:00+00:00 UTC  -- OK, <2h from now :) (now=2023-12-22T12:10:49+00:00 UTC)

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 parser python Pull requests that update Python code exchange config Pull request or issue for exchange configurations labels Dec 22, 2023
@silimotion
Copy link
Contributor Author

/review

Copy link
Contributor

PR Analysis

  • 🎯 Main theme: Adding a new exchange parser for CN->MN
  • 📝 PR summary: This PR introduces a new exchange parser for the CN->MN exchange. The parser fetches data from the Mongolian and Russian sources, assuming that Mongolia only trades with these two countries. The PR also includes updates to the relevant configuration files.
  • 📌 Type of PR: Enhancement
  • 🧪 Relevant tests added: No
  • ⏱️ Estimated effort to review [1-5]: 3, because the PR includes new logic for fetching and calculating exchange data, which requires careful review to ensure accuracy and reliability.
  • 🔒 Security concerns: No security concerns found

PR Feedback

  • 💡 General suggestions: The PR is generally well-structured and follows the project's conventions. However, it would be beneficial to add unit tests for the new exchange parser to ensure its correct functionality and prevent future regressions. Additionally, the assumption that Mongolia only trades with China and Russia should be clearly documented in the code to avoid confusion for future contributors.

  • 🤖 Code feedback:
    relevant fileparsers/MN.py
    suggestion      Consider handling the case where the 'datetime' or 'sortedZoneKeys' do not match between the two data sources. Currently, if these do not match, the data is silently ignored which could lead to inaccurate results. [important]
    relevant lineif (

    relevant fileparsers/MN.py
    suggestion      Consider adding error handling for the case where the 'importMW' key is not present in the 'query_data' dictionary. This would prevent potential KeyError exceptions. [medium]
    relevant linenetFlow=query_data["importMW"] + data["netFlow"],

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.

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.

We have already created some of this functionality to ensure it's consistent across parsers but it don't work until the RU.py parser has been upgraded. So I would like to hold of with this until that is upgraded.

(Some of the background for that is that we are doing a code freeze over the holidays anyhow so while we will be merging things in contrib they won't be deployed until early jan so there is no hurry to get this merged.)

parsers/MN.py Show resolved Hide resolved
@VIKTORVAV99
Copy link
Member

I could not get this to work when I tested it as no matching data could be found, I'll try again tomorrow.

zoneKey=sorted_zone_keys,
# We calculate the flow with the total imports and the MN->RU-2 exchange, as Mongolia has only two connections
netFlow=query_data["importMW"] + data["netFlow"],
source="https://ndc.energy.mn/",
Copy link
Member

Choose a reason for hiding this comment

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

The current implementation seems to be inversed.
Please double check this yourself.

It would also be good if we could add a test or two for this so we can validate the expected behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

seems to be inversed

I think it isn't.
Let's see:
CN->MN + RU->MN = Total Imports
CN->MN = Total Imports + MN->RU

I'll work on those tests. It would be optimal to be able to test the parser without requiring the sources to have matching data.

Copy link
Member

Choose a reason for hiding this comment

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

the easiest way would probably be to add snapshot tests using mock data. Like I did in #6285.

That way it don't rely on an online source, changing data or creating manual tests are are both logic intensive and hard to maintain long term.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking of doing something similar to what I did for the REE parser but I will take a look at these snapshot tests.

@VIKTORVAV99
Copy link
Member

@florianscheidl I just come to think about your project in relation to this. Would it be useful for you to know the total imports/exports in some form?

If so perhaps we can begin shaping the requirements for it as we don't store that at the moment? (MN is not the only zone that has this data in some form.)

@florianscheidl
Copy link
Member

Hey, @VIKTORVAV99, that would be interesting to pursue! Let's chat about it some time soon.

@github-actions github-actions bot added the tests label Feb 16, 2024
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 parser python Pull requests that update Python code tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants