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

feat(config): update zones .yaml + zone pydantic model to include region and country #6856

Merged
merged 5 commits into from
Jun 18, 2024

Conversation

Garsdal
Copy link
Contributor

@Garsdal Garsdal commented Jun 17, 2024

Issue

There is currently no clear source-of-truth for regions and countries of zones.

Description

  • Included a region and country attribute in the pydantic config model
  • Include region and country keys across all .yaml zone configurations

Notes on data sources

All regions and countries are following the ISO-3166 standard with values taken from here: https://github.com/lukes/ISO-3166-Countries-with-Regional-Codes/blob/master/all/all.csv

We want to use these new attributes for aggregation and grouping of our zones. It was also considered if this could be done using existing information, and I checked the subZoneNames. We do have the field sub_zone_names: list[ZoneKey] | None = Field(None, alias="subZoneNames") in our zone model, which indicates a parent_zone and sub_zone relation. However, this is not populated exhaustively across all configurations to be used instead of a country attribute.

Preview

An example of a US zone including country and region
image

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 python Pull requests that update Python code zone config Pull request or issue for zone configurations labels Jun 17, 2024
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.

I ran prettier to ensure the formatting stays the same and it passes the CI and all the changes look good now.

We would like to avoid merging more PRs until the latest frontend changes has been deployed tomorrow so I'll approve it then 🙂

@Garsdal Garsdal closed this Jun 18, 2024
@Garsdal Garsdal reopened this Jun 18, 2024
@Garsdal
Copy link
Contributor Author

Garsdal commented Jun 18, 2024

I ran prettier to ensure the formatting stays the same and it passes the CI and all the changes look good now.

We would like to avoid merging more PRs until the latest frontend changes has been deployed tomorrow so I'll approve it then 🙂

I will make a new commit today to try and fix the indentation :) I am not sure if it is breaking, but I still dislike that this PR changes 40.000 lines of code. And regarding the merge, I am in no hurry, but thanks for checking it so fast!

@VIKTORVAV99
Copy link
Member

VIKTORVAV99 commented Jun 18, 2024

I ran prettier to ensure the formatting stays the same and it passes the CI and all the changes look good now.

We would like to avoid merging more PRs until the latest frontend changes has been deployed tomorrow so I'll approve it then 🙂

I will make a new commit today to try and fix the indentation :) I am not sure if it is breaking, but I still dislike that this PR changes 40.000 lines of code. And regarding the merge, I am in no hurry, but thanks for checking it so fast!

Indentation should already be fixed 😉
And no worries! I prefer to take a look and merge the PRs as soon as possible if I can so they don't go stale for whatever reason.

value: 0.94
isRenewable:
unknown:
- datetime: '2019-01-01'
source: 'assumes 87% hydro, 5% biomass, 4% gas, 2% wind and 2% from various sources'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are these changes okay? Is it due to the formatting? @VIKTORVAV99

Copy link
Member

Choose a reason for hiding this comment

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

Perfectly fine, as long as it passes prettier it's good to go.

@Garsdal Garsdal marked this pull request as ready for review June 18, 2024 11:37
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.

LGMT! 🎉

@@ -77,3 +77,5 @@ fallbackZoneMixes:
unknown: 0.0
wind: 0.015505325244864603
timezone: Africa/Windhoek
region: Africa
country: null
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nulll?

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 decided to use null instead of Other for zones with an ambiguous country. It might potentially be a sensitive topic, so I thought it was the most appropriate to leave the value as null in these cases. Please bring other suggestions if you have any!

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that's a nice approach but this is Namibia and as far as I know it's not contested?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, you're right, good catch! I don't know how that snuck a null value there, but i'll update it to the correct value. Appreciate the nice check :)

Copy link
Collaborator

@tonypls tonypls left a comment

Choose a reason for hiding this comment

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

Just the one small comment but otherwise LGTM

@Garsdal Garsdal merged commit 04d1260 into master Jun 18, 2024
20 checks passed
@Garsdal Garsdal deleted the mlg/update-zones-config-yaml branch June 18, 2024 12:23
Comment on lines 1 to +3
timezone: America/Nuuk
region: Americas
country: GL
Copy link
Member

Choose a reason for hiding this comment

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

As a fellow dane I am very disappointed in you on this one 😉 🇬🇱

Copy link
Member

Choose a reason for hiding this comment

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

Oh don't you start or we'll have a mess on our hands with all these overseas territories... 😅

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 used the ISO-3166-1 (https://github.com/lukes/ISO-3166-Countries-with-Regional-Codes/blob/master/all/all.csv) standard to generate the configs, but it hurts my danish heart 😭

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

4 participants