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

updated FR parser to enable historical backfill #5240

Merged
8 commits merged into from
Mar 28, 2023
Merged

Conversation

ghost
Copy link

@ghost ghost commented Mar 27, 2023

Issue

We noticed that we couldn't refetch data that was older than 9 months using the current dataset. We know that RTE publishes final data at Y+1 and we want to be able to update our dataset with the most up-to-date data.
image

Description

the aim here is to enable historical refetch. the historical data is available in the dataset eco2mix-national-cons-def

Preview

2023-03-26
image

2021-12-31

image

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.

@ghost ghost requested review from VIKTORVAV99, unitrium and wobniarin March 27, 2023 15:55
@github-actions github-actions bot added parser zone config Pull request or issue for zone configurations labels Mar 27, 2023
@netlify
Copy link

netlify bot commented Mar 27, 2023

Deploy Preview for phenomenal-syrniki-c5ceaa ready!

Name Link
🔨 Latest commit a16ed21
🔍 Latest deploy log https://app.netlify.com/sites/phenomenal-syrniki-c5ceaa/deploys/6421c56702df340008016e52
😎 Deploy Preview https://deploy-preview-5240--phenomenal-syrniki-c5ceaa.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

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.

Note this is not a review of the code itself but the implementation of the new data source. In #4853 there was some concerns about validating the data source and making sure all the data was valid before we proceeded. Is this still the case?

If not might it not be better to proceed with #4853 and split France up in the process when switching the parsers (and aggregating it again together with corsica)?

@ghost
Copy link
Author

ghost commented Mar 28, 2023

If not might it not be better to proceed with #4853 and split France up in the process when switching the parsers (and aggregating it again together with corsica)?

Hi @VIKTORVAV99, I don't know how much we want to prioritize splitting FR into regions. We realised we couldn't backfill and that the data can be significantly modified and thought that this was more important to the Sales team. As you can see on the OpenData page, the data we collect is provisional.

@VIKTORVAV99
Copy link
Member

If not might it not be better to proceed with #4853 and split France up in the process when switching the parsers (and aggregating it again together with corsica)?

Hi @VIKTORVAV99, I don't know how much we want to prioritize splitting FR into regions. We realised we couldn't backfill and that the data can be significantly modified and thought that this was more important to the Sales team. As you can see on the OpenData page, the data we collect is provisional.

Perhaps we should increase the priority of https://github.com/electricitymaps/electricitymaps/pull/3230, since it would allow us to fetch estimated data (separate from validated data)? Seems like yet another scenario where it would be beneficial.

I'll do a full code review this afternoon as well 👍🏻

Copy link
Contributor

@unitrium unitrium left a comment

Choose a reason for hiding this comment

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

Just some naming suggestions but looks good to me!

parsers/FR.py Outdated Show resolved Hide resolved
parsers/FR.py Outdated Show resolved Hide resolved
@VIKTORVAV99 VIKTORVAV99 self-requested a review March 28, 2023 08:36
parsers/FR.py Outdated Show resolved Hide resolved
Mathilde Daugy and others added 3 commits March 28, 2023 10:41
Co-authored-by: Robin TROESCH <38283096+unitrium@users.noreply.github.com>
Co-authored-by: Robin TROESCH <38283096+unitrium@users.noreply.github.com>
parsers/FR.py Outdated Show resolved Hide resolved
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.

Everything looks good to me with the exception of the type issue. If you can take care of that it would be greatly appreciated.

@ghost ghost merged commit a38a1e7 into master Mar 28, 2023
@ghost ghost deleted the md/update_FR_parser branch March 28, 2023 09:25
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
parser 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