Standardize country codes iso 3166#159
Standardize country codes iso 3166#159ram-from-tvl wants to merge 10 commits intoopenclimatefix:mainfrom
Conversation
|
Hi @peterdudfield |
|
All tests have passed. |
There was a problem hiding this comment.
Pull request overview
This PR aims to standardize country code parameters across the solar-consumer project to ISO 3166-1 alpha-3-style codes (with special handling like gbr_gb), updating application defaults, dispatch logic, and tests/docs accordingly.
Changes:
- Updated country-code parameters/defaults across fetch, save, and app orchestration paths (e.g.
gb→gbr_gb,nl→nld,de→deu,be→bel,ind_rajasthan→ind_rj). - Updated unit/integration tests to use the new country code strings.
- Updated user-facing documentation and example environment configuration to reflect the new codes.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
solar_consumer/fetch_data.py |
Updates default country and dispatch table keys to new codes; updates docstrings/messages. |
solar_consumer/app.py |
Updates default country and country→model_tag mapping to new codes. |
solar_consumer/save/save_data_platform.py |
Updates default country and NL/GB branching comparisons to new codes. |
solar_consumer/save/save_site_database.py |
Updates defaults and country comparisons/docstrings for site-db saving logic. |
solar_consumer/data/fetch_gb_data.py |
Adds fallback logic for missing GSPs (DB-backed) and adjusts handling of empty GSP frames. |
tests/unit/test_fetch_data.py |
Updates GB historic tests to call fetch_data(..., country="gbr_gb"). |
tests/unit/test_save_forecast.py |
Updates NL and India country args used in save tests. |
tests/integration/test_save_nl_to_data_platform.py |
Updates NL integration test to use country="nld". |
README.md |
Updates COUNTRY env var docs to new codes. |
.example.env |
Updates default COUNTRY value and comment options list. |
Comments suppressed due to low confidence (1)
solar_consumer/save/save_data_platform.py:56
save_generation_to_data_platformtreats anycountryother than "nld" as the GB branch. Now that the application supports other country codes (e.g. "deu", "bel", "ind_rj"), calling this with those values (e.g. viaapp.pywhenSAVE_METHOD=data-platform) will incorrectly assume GB columns likegsp_id/regimeexist and can fail or silently write bad data. Add explicit validation (raise for unsupported countries) or implement dedicated branches per supported country.
# 0. Create the observers required if they don't exist already
if country == "nld":
required_observers = {"nednl"}
id_key = "region_id"
capacity_col = "capacity_kw"
capacity_multiplier = 1000
else: # gbr_gb
required_observers = {"pvlive_in_day", "pvlive_day_after"}
id_key = "gsp_id"
capacity_col = "capacity_mwp"
capacity_multiplier = 1e6
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| If `capacity_override_kw` is provided, that value will be used when creating the site. | ||
| For Germany, the TSO’s known capacity is used if no override is given. For NL, default 20GW applied | ||
|
|
||
| Parameters: | ||
| session (Session): CurrentSQLAlchemy session | ||
| pvsite (PVSite): Pydantic model with site metadata | ||
| country (str): Country code ('nl' or 'de') | ||
| country (str): Country code ('nld' or 'deu') | ||
| capacity_override_kw (Optional[int]): Force a specific capacity on creation |
There was a problem hiding this comment.
The get_or_create_pvsite docstring says country is ('nld' or 'deu'), but the function is also used for India Rajasthan via save_generation_to_site_db(... country="ind_rj"). Update the docstring (and/or the capacity-selection comment) to include the supported ind_rj code so callers know it’s valid.
There was a problem hiding this comment.
@ram-from-tvl check can we please check this i think we can update the doc string here
README.md
Outdated
|
|
||
| - `DB_URL=postgresql://postgres:postgres@localhost:5432/neso_solar` : Database Configuration | ||
| - `COUNTRY="gb"` : Country code for fetching data. Currently, other options are ["be", "ind_rajasthan", "nl"] | ||
| - `COUNTRY="gbr_gb"` : Country code for fetching data. Currently, other options are ["bel", "ind_rj", "nld"] |
There was a problem hiding this comment.
README COUNTRY documentation lists other options as ["bel", "ind_rj", "nld"], but the application also supports Germany ("deu") (see app.py country→model_tag mapping and fetch_data dispatch table). Update the README options list to include "deu" (and any other supported codes) to avoid misconfiguration.
| - `COUNTRY="gbr_gb"` : Country code for fetching data. Currently, other options are ["bel", "ind_rj", "nld"] | |
| - `COUNTRY="gbr_gb"` : Country code for fetching data. Currently, other options are ["bel", "deu", "ind_rj", "nld"] |
|
@katyalmohit and @pavankulkarni007 could you review this? |
|
Hi @peterdudfield |
Up to you, if you think they are good, use them. If you dont think they are good, perhaps reject them |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Reverts fetch_gb_data.py to upstream state. The backup GSP data logic (DB_URL lookup, LocationSQL queries, national->GSP data synthesis) falls outside the scope of the country-code standardization issue (openclimatefix#157). This will be addressed in a separate PR.
|
Hey @PavanRaghavendraKulkarni, You're right. the backup GSP data logic was out of scope for this PR. I've now:
Thanks for the review feedback! |
| If `capacity_override_kw` is provided, that value will be used when creating the site. | ||
| For Germany, the TSO’s known capacity is used if no override is given. For NL, default 20GW applied | ||
|
|
||
| Parameters: | ||
| session (Session): CurrentSQLAlchemy session | ||
| pvsite (PVSite): Pydantic model with site metadata | ||
| country (str): Country code ('nl' or 'de') | ||
| country (str): Country code ('nld' or 'deu') | ||
| capacity_override_kw (Optional[int]): Force a specific capacity on creation |
There was a problem hiding this comment.
@ram-from-tvl check can we please check this i think we can update the doc string here
README.md
Outdated
|
|
||
| - `DB_URL=postgresql://postgres:postgres@localhost:5432/neso_solar` : Database Configuration | ||
| - `COUNTRY="gb"` : Country code for fetching data. Currently, other options are ["be", "ind_rajasthan", "nl"] | ||
| - `COUNTRY="gbr_gb"` : Country code for fetching data. Currently, other options are ["bel", "ind_rj", "nld"] |
|
@PavanRaghavendraKulkarni Thanks for the review! I've addressed both unresolved comments:
|
- Updated get_or_create_pvsite docstring to include 'ind_rj' as supported country code - Added 'deu' to README COUNTRY environment variable options list
- Resolved conflicts in save_data_platform.py: accepted upstream's refactored config-driven structure, applied ISO 3166 country codes (nl→nld, be→bel, gb→gbr_gb) - Accepted upstream deletion of test_save_nl_to_data_platform.py (replaced by test_save_nl_and_be_to_data_platform.py) - Updated locations.csv country_code column: nl→nld, be→bel - Updated test_save_nl_and_be_to_data_platform.py with new country codes - Updated test_save_be_generation_unit.py with new country codes
|
@ram-from-tvl PR lGTM! please resolve the merge conflicts |
Pull Request
Description
This PR standardizes all country codes in the solar-consumer project to follow the ISO 3166-1 alpha-3 standard, as requested in issue #157.
Changes made:
gbgbr_gbnlnlddedeubebelind_rajasthanind_rjFiles updated:
os.getenvdefaultcountry=argument in GB historic testscountry=argument for NL and India testscountry=argumentCOUNTRYenvironment variable documentationCOUNTRYvalue and available options commentOnly country code parameter strings were changed. No data-level identifiers (e.g.
"nl_national"site names,"Belgium"region names from the Elia API) were modified — these are data values, not country code parameters.Fixes #157
Checklist: