Standardize country codes iso 3166#159
Standardize country codes iso 3166#159ram-from-tvl wants to merge 5 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.
| logger.info(f"Created backup data for {len(backup_rows)} entries") | ||
| except Exception as e: | ||
| logger.error(f"Error creating backup GSP data: {e}") | ||
|
|
There was a problem hiding this comment.
fetch_gb_data_historic now only appends non-empty frames to all_gsps_yields, but still unconditionally returns pd.concat(all_gsps_yields, ...). If PVLive returns empty data for all queried GSPs (e.g., upstream outage), all_gsps_yields will be empty and pd.concat will raise ValueError: No objects to concatenate. Handle the empty-list case by returning an empty DataFrame with the expected columns (or by always appending an empty frame).
| if not all_gsps_yields: | |
| # Return an empty DataFrame with the expected columns if no data was fetched | |
| return pd.DataFrame( | |
| columns=[ | |
| "target_datetime_utc", | |
| "solar_generation_kw", | |
| "gsp_id", | |
| "installedcapacity_mwp", | |
| "capacity_mwp", | |
| "regime", | |
| "pvlive_updated_utc", | |
| ] | |
| ) |
| if missing_gsps and national_df is not None: | ||
| logger.info(f"Creating backup data for {len(missing_gsps)} missing GSPs using national data") | ||
| db_url = os.getenv("DB_URL") | ||
| if not db_url: | ||
| logger.warning("DB_URL not set, cannot create backup GSP data") | ||
| else: | ||
| try: | ||
| connection = DatabaseConnection(url=db_url) | ||
| with connection.get_session() as session: | ||
| locations = session.query(LocationSQL).filter(LocationSQL.gsp_id.in_(missing_gsps)).all() |
There was a problem hiding this comment.
This adds new behavior beyond country-code standardization: when GSPs are missing it attempts to connect to the database (via DB_URL) and synthesize backup per-GSP data from national data and LocationSQL capacities. This contradicts the PR description claim that only country-code parameter strings changed, and it also introduces a runtime dependency on DB connectivity during data fetching. Either remove this change from the PR or update the PR description/scope and add tests/guardrails for this new backup path.
| 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.
|
|
||
| :param country: "gb", "nl", "de", "ind_rajasthan", or "be" | ||
| :param country: "gbr_gb", "nld", "deu", "ind_rj", or "bel" | ||
| :param historic_or_forecast: "generation" or "forecast" |
There was a problem hiding this comment.
fetch_data docstring says historic_or_forecast is "generation" or "forecast", but the codebase uses other values too (e.g. GB historic tests pass "historic"; India fetcher defaults to "historic" and ignores it). This makes the public API misleading. Document the actual accepted values (or normalize/validate them in fetch_data before dispatch).
| :param historic_or_forecast: "generation" or "forecast" | |
| :param historic_or_forecast: Indicator passed through to the country-specific fetcher, e.g. "forecast", | |
| "generation", or "historic". The exact accepted values are backend-specific; see the corresponding | |
| fetch_<country>_data function for details. |
|
|
||
| - `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"] |
|
|
||
|
|
There was a problem hiding this comment.
app() assigns model_tag only for a fixed set of countries and has no else/default. If an unsupported country value is provided (e.g. via COUNTRY env var), model_tag will be undefined and the function can crash later with UnboundLocalError. Add an explicit else that raises a clear exception listing supported country codes.
| else: | |
| supported_countries = ["gbr_gb", "nld", "deu", "bel"] | |
| raise ValueError( | |
| f"Unsupported country code: {country!r}. " | |
| f"Supported country codes are: {', '.join(supported_countries)}." | |
| ) |
1 participant
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: