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

French regions #4853

Closed
wants to merge 43 commits into from
Closed

French regions #4853

wants to merge 43 commits into from

Conversation

corradio
Copy link
Member

@corradio corradio commented Dec 12, 2022

Issue

#2777

Description

French regional data pulling directly from ECO2MIX. Uses the same data as displayed in the RTE app.
⚠️ Lumps all thermal into unknown as thermal breakdown is unknown. This affects the country as well, as it is computed as an aggregation of subzones.

Outstanding issues

  • Working production parser
  • Regional geometries, see web/third_party_maps/*.geojson [need help]
  • Update bounding boxes
  • Delete geojson files
  • Parser needs to support fetching exchanges
  • Shallow data validation (total production is conserved across aggregation)
  • Deeper data validation

Data validation

TBD.

Preview

@corradio corradio marked this pull request as draft December 12, 2022 20:42
@github-actions github-actions bot added exchange config Pull request or issue for exchange configurations frontend 🎨 parser zone config Pull request or issue for zone configurations labels Dec 12, 2022
@corradio
Copy link
Member Author

Seems like we're mostly done! Would still require data validation though

image

@VIKTORVAV99
Copy link
Member

I would suggest we keep the 3rd party maps in the PR (and repository) for now until we are happy with both the zone and aggregated geometries.

@VIKTORVAV99 VIKTORVAV99 linked an issue Dec 15, 2022 that may be closed by this pull request
@corradio corradio marked this pull request as ready for review December 16, 2022 07:56
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.

Looks good for the most part, just some small notes and one question.

In the current return objects it returns nan, should this not be Null/None if it's missing?

parsers/ECO2MIX.py Outdated Show resolved Hide resolved
parsers/ECO2MIX.py Outdated Show resolved Hide resolved
"France": "FR",
**{
subzone.replace("FR-", ""): subzone
for subzone in ZONES_CONFIG["FR"].get("subZoneNames", [])
Copy link
Member

Choose a reason for hiding this comment

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

This creates some type errors as the Literal "FR" can't be assigned to the ZoneKey type but should not affect the functionality.

}


def query(url_type_arg, session=None, target_datetime=None):
Copy link
Member

Choose a reason for hiding this comment

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

Could be smart to add types here (and the other functions) for better linting and DX but it's technically not required.

parsers/ECO2MIX.py Outdated Show resolved Hide resolved
corradio and others added 3 commits December 16, 2022 13:00
Co-authored-by: Viktor Andersson <[email protected]>
Co-authored-by: Viktor Andersson <[email protected]>
Co-authored-by: Viktor Andersson <[email protected]>
@corradio
Copy link
Member Author

Looks good for the most part, just some small notes and one question.

In the current return objects it returns nan, should this not be Null/None if it's missing?

From what I know, nans will be turned to NULL before being inserted in the database (I tested locally without our backend). I can change that if needed though.

@VIKTORVAV99
Copy link
Member

Looks good for the most part, just some small notes and one question.
In the current return objects it returns nan, should this not be Null/None if it's missing?

From what I know, nans will be turned to NULL before being inserted in the database (I tested locally without our backend). I can change that if needed though.

Then it should be fine to keep as is. 🙂

@unitrium
Copy link
Contributor

Just putting some thoughts here as we are trying to achieve something similar in #4894.
Do we want to make the FR zone an aggregated zone ? Does this make sense since we have the capability of getting the data directly at a national level ?

In a perfect world our aggregated data should match the data from the national feed but high chances of having some differences.
I'm thinking we should keep the national data feed as well and add something to the country config like contryView: true to have it only displayed for the country level view as a replacement.

@@ -290,3 +290,16 @@ sources:
: link: https://unece.org/sites/default/files/2022-04/LCA_3_FINAL%20March%202022.pdf#page=37,
https://proceedings.windeurope.org/biplatform/rails/active_storage/disk/eyJfcmFpbHMiOnsibWVzc2FnZSI6IkJBaDdDRG9JYTJWNVNTSWhORFJ0ZDJJMWVUbG9OMll6TVRaaGEza3lkamgxZG1aM056WnZZZ1k2QmtWVU9oQmthWE53YjNOcGRHbHZia2tpQVk1cGJteHBibVU3SUdacGJHVnVZVzFsUFNKWGFXNWtaWFZ5YjNCbExWZHBibVF0Wlc1bGNtZDVMV2x1TFVWMWNtOXdaUzB5TURJeExYTjBZWFJwYzNScFkzTXVjR1JtSWpzZ1ptbHNaVzVoYldVcVBWVlVSaTA0SnlkWGFXNWtaWFZ5YjNCbExWZHBibVF0Wlc1bGNtZDVMV2x1TFVWMWNtOXdaUzB5TURJeExYTjBZWFJwYzNScFkzTXVjR1JtQmpzR1ZEb1JZMjl1ZEdWdWRGOTBlWEJsU1NJVVlYQndiR2xqWVhScGIyNHZjR1JtQmpzR1ZBPT0iLCJleHAiOiIyMDIyLTExLTAyVDE1OjU0OjAzLjEyNFoiLCJwdXIiOiJibG9iX2tleSJ9fQ==--c25280a7345257bd91bfaf6d64ddb75c55eef799/Windeurope-Wind-energy-in-Europe-2021-statistics.pdf?content_type=application%2Fpdf&disposition=inline%3B+filename%3D%22Windeurope-Wind-energy-in-Europe-2021-statistics.pdf%22%3B+filename%2A%3DUTF-8%27%27Windeurope-Wind-energy-in-Europe-2021-statistics.pdf
timezone: Europe/Paris
subZoneNames:
Copy link
Contributor

Choose a reason for hiding this comment

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

Just wondering, how will the flowtracing pipeline react?
It will see data from France from both directly measured and also compute the aggregation data. So I guess we will have two datapoints for the same timestamp. One will probably replace the other as we are indexing by datetime. Since the aggregation is computed last, it will probably be the aggregation that will be in the database.

Copy link
Member

Choose a reason for hiding this comment

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

Could you check how it works for Sweden? It should both have working parsers and Aggregates the subzones.

Note: for Sweden the source is exactly the same as we sum the bidding zones in the ENTSO-E parser.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok thanks! I didn't know we were producing the SE data like this.
So the way I see it, the last step of the flowtracing pipeline computes spatial aggregations, which I think would be inserted after the flowtracing of parsed data. Therefore if a flowtraced data from parsed data exists with the same timestamp as the flowtraced data coming from the spatial aggregation step, it will be overwritten.

Copy link
Contributor

Choose a reason for hiding this comment

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

So as it stands I think having a main zone defined like this would most likely overwrite the data obtained through the parsers and have it replaced with data obtained through the aggregation step. Is it the intended behavior ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Our aggregations should give the same as the national level (especially in the case of Sweden since we are doing it at parser level). But there might be differences we would need to explain.

Copy link
Member

Choose a reason for hiding this comment

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

So as it stands I think having a main zone defined like this would most likely overwrite the data obtained through the parsers and have it replaced with data obtained through the aggregation step.

We could remove the parser level aggregation for Sweden and see what happens? ENTSOE-E has country level data as well but it was missing solar and gas but it would be nice to do this anyway as it would save us a few API calls.

Copy link
Member Author

Choose a reason for hiding this comment

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

@unitrium yes it's correct that the country-wide data will be overwritten by the aggregation if the subzones pass the quality criterion (for now we just check if all subzones have been flowtraced). We could change the quality check to make sure the country load is equal to the aggregated load.

@netlify
Copy link

netlify bot commented Feb 8, 2023

Deploy Preview for phenomenal-syrniki-c5ceaa ready!

Name Link
🔨 Latest commit 471abc1
🔍 Latest deploy log https://app.netlify.com/sites/phenomenal-syrniki-c5ceaa/deploys/63e36a61830a640008a6907b
😎 Deploy Preview https://deploy-preview-4853--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.

@VIKTORVAV99
Copy link
Member

@corradio I just merged master into this branch and will try and get the geometries updated today but is there anything else blocking this from getting merged?

@corradio
Copy link
Member Author

corradio commented Feb 8, 2023 via email

@VIKTORVAV99 VIKTORVAV99 added the BLOCKED Waiting on other task or external factors label Feb 8, 2023
@ghost ghost mentioned this pull request Mar 28, 2023
2 tasks
@VIKTORVAV99
Copy link
Member

Is this still blocked?

If so I think we should close the PR until it becomes unblocked.

@VIKTORVAV99
Copy link
Member

Closing this for now as there is more and more merge conflicts, it would most likely be easier to start from scratch than resolving all the merge conflicts.

@VIKTORVAV99 VIKTORVAV99 deleted the olc/split-FR branch March 2, 2024 17:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BLOCKED Waiting on other task or external factors exchange config Pull request or issue for exchange configurations frontend 🎨 parser translations 🗣 zone config Pull request or issue for zone configurations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

France: mismatch between electricitymaps and RTE data Data for France (regional)
4 participants