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

remove all vietnams subzones #7705

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

silkeholmebonnen
Copy link
Contributor

Issue

GMM-249

Description

Remove VN-N, VN-S and VN-C.
Following the first step of this guide to delete all references in contrib

Preview

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.

@silkeholmebonnen silkeholmebonnen requested review from a team and VIKTORVAV99 as code owners January 8, 2025 13:30
@github-actions github-actions bot added parser translations 🗣 python Pull requests that update Python code zone config Pull request or issue for zone configurations labels Jan 8, 2025
@VIKTORVAV99
Copy link
Member

Will try and take a look later today but do we really want to remove the parser as it fetches consumption data and price?

We can just move the zone configs to the archived zones and keep it running, then we'll keep getting data we can use later without them being tied together to the main VI zones, we do this for some old India zones already.

@madsnedergaard
Copy link
Member

Looks fine on my side, but before merging this we need to do something similar to this open PR https://github.com/electricitymaps/electricitymaps/pull/6219 on the internal API side (e.g. if calling VI-X, set VI as zone) to ensure calling the subzones does not break the API :)

@VIKTORVAV99
Copy link
Member

VIKTORVAV99 commented Jan 8, 2025

Looks fine on my side, but before merging this we need to do something similar to this open PR electricitymaps/electricitymaps#6219 on the internal API side (e.g. if calling VI-X, set VI as zone) to ensure calling the subzones does not break the API :)

@Mads

Looks fine on my side, but before merging this we need to do something similar to this open PR electricitymaps/electricitymaps#6219 on the internal API side (e.g. if calling VI-X, set VI as zone) to ensure calling the subzones does not break the API :)

@madsnedergaard what is the general policy around this, should we always do it for all zones or just zones we have data for or have had in the past?

Yes it will technically break the API but they won't get more or less data than before regardless.

I will be merging Mexico as well next week so would be good to know.

@madsnedergaard
Copy link
Member

Looks fine on my side, but before merging this we need to do something similar to this open PR electricitymaps/electricitymaps#6219 on the internal API side (e.g. if calling VI-X, set VI as zone) to ensure calling the subzones does not break the API :)

@madsnedergaard what is the general policy around this, should we always do it for all zones or just zones we have data for or have had in the past?

Yes it will technically break the API but they won't get more or less data than before regardless.

I will be merging Mexico as well next week so would be good to know.

I'd say that as long as it's been published on /zones at some point, we should attempt to handle it gracefully whenever possible. Dunno if I'm overthinking it though 😅

@VIKTORVAV99
Copy link
Member

Looks fine on my side, but before merging this we need to do something similar to this open PR electricitymaps/electricitymaps#6219 on the internal API side (e.g. if calling VI-X, set VI as zone) to ensure calling the subzones does not break the API :)

@madsnedergaard what is the general policy around this, should we always do it for all zones or just zones we have data for or have had in the past?
Yes it will technically break the API but they won't get more or less data than before regardless.
I will be merging Mexico as well next week so would be good to know.

I'd say that as long as it's been published on /zones at some point, we should attempt to handle it gracefully whenever possible. Dunno if I'm overthinking it though 😅

Please just promise me you'll kill all these in the V4 API and I'll do it 🙂

@silkeholmebonnen
Copy link
Contributor Author

Will try and take a look later today but do we really want to remove the parser as it fetches consumption data and price?

We can just move the zone configs to the archived zones and keep it running, then we'll keep getting data we can use later without them being tied together to the main VI zones, we do this for some old India zones already.

We can keep the parser. Would you have time to do it though? I don't plan on working until mid/end of next week.

Also, the remove script delete dthe zones from the world.geojson, but since VN is not defined by itself, VN is now not on the map. What's the easiest way to go about this? I tried to draw VN in QGIS following our wiki but its taking so long to get right. @VIKTORVAV99

Looks fine on my side, but before merging this we need to do something similar to this open PR electricitymaps/electricitymaps#6219 on the internal API side (e.g. if calling VI-X, set VI as zone) to ensure calling the subzones does not break the API :)

I have a backend PR open, so I can do it mid/end of next week but feel free to do it for me @madsnedergaard

@VIKTORVAV99
Copy link
Member

Will try and take a look later today but do we really want to remove the parser as it fetches consumption data and price?

We can just move the zone configs to the archived zones and keep it running, then we'll keep getting data we can use later without them being tied together to the main VI zones, we do this for some old India zones already.

We can keep the parser. Would you have time to do it though? I don't plan on working until mid/end of next week.

Also, the remove script delete dthe zones from the world.geojson, but since VN is not defined by itself, VN is now not on the map. What's the easiest way to go about this? I tried to draw VN in QGIS following our wiki but its taking so long to get right. @VIKTORVAV99

Looks fine on my side, but before merging this we need to do something similar to this open PR electricitymaps/electricitymaps#6219 on the internal API side (e.g. if calling VI-X, set VI as zone) to ensure calling the subzones does not break the API :)

I have a backend PR open, so I can do it mid/end of next week but feel free to do it for me @madsnedergaard

I can take care of the map and such on Monday as I'll be doing some other map changes as well.

The other stuff I can probably do later today or tomorrow.

@consideRatio
Copy link
Contributor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
parser python Pull requests that update Python code translations 🗣 zone config Pull request or issue for zone configurations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants