Avoid dropping generators with empty time series when clips equal zero #714
Conversation
|
@ekatef would you mind reviewing this PR? :) |
Looks very elegant! Tomorrow I'll be absolutely happy to review :) |
scripts/add_electricity.py
Outdated
| inflow_stations = bus_id[inflow_idx] | ||
| missing_c = pd.Index(inflow_stations.unique()).difference( | ||
| inflow.indexes["plant"] | ||
| ) | ||
| intersection_c = inflow.indexes["plant"].intersection(inflow_stations) | ||
|
|
||
| # if missing time series are found, notify the user and exclude missing hydro plants | ||
| if not missing_c.empty: | ||
| idxs_to_keep = inflow_stations[ | ||
| inflow_stations.isin(intersection_c) | ||
| ].index | ||
| ror = ror.loc[ror.index.intersection(idxs_to_keep)] | ||
| hydro = hydro.loc[hydro.index.intersection(idxs_to_keep)] |
There was a problem hiding this comment.
A very nice and clean solution by its essence. What has been not easy to understand for me when reading the code are the naming conventions. A couple of suggestions:
- Is it possible to clarify in the names what does
"_c"imply inmissing_candintersection_c? - Usually we refer generators as power plants, while the
"stations"is used in variables names related to a power plants dataset, whilestationsmight have implications with meteorological measurements. Would it be worth to revise naming a bit?
There was a problem hiding this comment.
Revised:
- _c -> _plants
- stations -> buses
scripts/add_electricity.py
Outdated
| logger.warning( | ||
| f"'{snakemake.input.profile_hydro}' is missing " | ||
| f"inflow time-series for at least one bus: {', '.join(missing_c)}. Corresponding hydro plants are dropped." |
There was a problem hiding this comment.
Would it be possible to output also some data on the missed power plants like coordinates or names along with buses ids?
scripts/add_electricity.py
Outdated
| logger.warning( | ||
| "Unexpected missing 'weight' column; typical when no generators are detected. Manually added." | ||
| ) |
There was a problem hiding this comment.
I'd say rather "it could happen" as "typical" can be misleading. What do you think?
scripts/build_renewable_profiles.py
Outdated
| # if technology is not hydro, restrict the region of the cutout | ||
| # hydrobasins may span beyond the region of the country, so | ||
| # it is unsafe to restrict the region for hydro |
There was a problem hiding this comment.
Could it make sense to re-formulate a bit the comment for better readability? Something like "the region should be restricted for non-hydro technologies, while hydro potential is calculated across hydrobasins which may span beyond the region of the country"?
|
@ekatef Revised :)
|
Super!! With the revisions implemented, I'm able to recall the coded logic significantly faster ;) Thanks for the clarification regarding merge. Feels like a bit difficult to formalise, but quite clear when you explain reasoning behind |
* adapt #714 to avoid hydro mismatch * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Closes #704, #706
Changes proposed in this Pull Request
This PR co-authored with @ekatef that identified the issue address the problem of no generators being found and no weight column to be found.
This issue may complement the PR #709.
Checklist
envs/environment.yamlandenvs/environment.docs.yaml.config.default.yamlandconfig.tutorial.yaml.test/(note tests are changing the config.tutorial.yaml)doc/configtables/*.csvand line references are adjusted indoc/configuration.rstanddoc/tutorial.rst.doc/release_notes.rstis amended in the format of previous release notes, including reference to the requested PR.