Review and refactor before merging develop with main by aodenweller · Pull Request #15 · pik-piam/IAM-PyPSA-coupling

aodenweller · 2026-07-27T14:08:07Z

This PR addresses code issues that surfaced when comparing the develop branch to the main branch in this PR: #13

Changes

Move prepare_capacities and build_capacity_targets from capacities.py into base.py as it requires a loader and therefore shouldn't belong under transforms (breaking)
- This was the only outlier left in transforms that took a loader, now fixed.
New apply_currency_factor function used on all currency cost parameters (investment, VOM, fuel price, CO2 price)
Update function names:
- discount_rates -> build_discount_rates
- assemble_variable_set -> build_variable_set
Data reading
- Move some code from couplers.base.discount_rates to costs.select_discount_rate
- Cache _read_iamc_variables, used in list_iamc_variables
- Simplify how sectoral demand is read from mif files
Comments/docstrings
- Remove "tidy frame" blah
- Replace "rebase" with "convert from FE to SE"
- Shorten several docstrings that were too verbose
CI/CD stuff
- Include test.yml workflow, restricted to develop and main branches
- Make sure remind/regions.csv is shipped with package once deployed
Clean-up:
- Remove deprecated conftest.py
- Remove convert_co2_prices: unit conversion occurs when loading, currency conversion occurs in apply_currency_factor
- Remove unit_factor from loads.py: unit conversion occurs when loading
- Remove deprecated ssp_population and ssp_gdp properties for Coupler
- Remove PyPSA-Eur and REMIND from comments
- Always use "USD" instead of "$"
Update all tests (thanks to Claude)

To be decided

To be decided @irr-github (also see remaining comments in Rewrite package #13)
- ~~Can we delete the IAMPYPSA_SYMBOLS override functionality in remind_symbols.py?~~ --> Yes
- ~~Can we delete validate_scenario_against_remind?~~ --> Yes, to be replaced by something else later once coupling other IAMs.
- ~~Can we delete read_gdx_scalar in gdx.py and load_scalar in loader.py?~~ --> No, keep for now for metadata.
- ~~Can we delete src/dev/pypsa_copies.py?~~--> Yes
- ~~Can we delete the deprecated build_ssp_shares function?~~ --> Yes
- ~~Can we delete the IAMPYPSA_SSP_API_URL environment variable handling in ssp.py?~~ --> Yes
ToDo later (I'll follow up in a another PR)
- Some functions to be renamed (w/o action verb)
- Docstrings are inconsistent across package

- Updated `remind_symbols_gdx.yaml` and `remind_symbols_mif.yaml` - Enhanced `_source_unit` function in `remind_symbols.py` to prioritize live unit data from MIF files, raising errors for mismatches with declared units. - Introduced `apply_currency_factor` in `costs.py` to scale currency-denominated parameters. - Remove `ssp_population` and `ssp_gdp` from coupler. - Rename `discount_rates` to `build_discount_rates`, move part of it into costs.py for consistency

- Updated documentation in architecture and technology mapping - Remove `unit_factor` in `build_regional_demand` - Remove `eta_exponents` in `convert_investment_to_input_capacity_basis` - Remove any mention of "tidy frames" throughout - Update docstrings

…data handling

- Move `prepare_capacities` and `build_capacity_targets` from `capacities.py` into `base.py` as it requires a loader and therefore shouldn't belong under transforms. - Remove `convert_co2_prices`: unit conversion occurs when loading, currency conversion occurs in `apply_currency_factor` - Cache `_read_iamc_variables` for listing IAMC variables - Update tests

irr-github

Thanks for going carefully through this! Some great improvements. It also exposes some potential design flaws we need to discuss.

One negative: the changes to transform.loads and transform.costs mean they are no longer transforms. I would either roll back or migrate entirely

irr-github · 2026-08-03T09:33:14Z

    if: github.event_name == 'release' || (github.event_name == 'workflow_dispatch' && github.event.inputs.publish == 'true') || startsWith(github.ref, 'refs/tags/')
    environment:
      name: pypi
-      url: https://pypi.org/p/REMIND-PyPSA-coupling


this looks fake! I propose to delete the release.yml action (broken anyway) until we are ready for pypi

irr-github · 2026-08-03T09:34:34Z

+
+      - name: Install the package and test dependencies
+        run: |
+          python -m pip install --upgrade pip


what about

- uses: astral-sh/setup-uv@v6 with: python-version: "3.12"

irr-github · 2026-08-03T09:35:45Z

  supplies the per-region discount rate, merged into the cost table alongside these components.
  For consistency with the IAM, additional technologies may need to be added on the PyPSA side.
+  IAM-sourced monetary values (investment/VOM/fuel) are scaled by the config's `currency_factor`
+  (default `1.0`, a no-op) to convert REMIND's USD output into the PyPSA baseline's currency —


for pypsa-eur to remind I assume this directly converts EUR2015 to USD2017 no? if so the comment is possibly misleading

irr-github · 2026-08-03T09:49:21Z

+  [Technology mapping](technology-mapping.md)). A separate `Coupler.build_discount_rates(year)` method
  supplies the per-region discount rate, merged into the cost table alongside these components.
  For consistency with the IAM, additional technologies may need to be added on the PyPSA side.
+  IAM-sourced monetary values (investment/VOM/fuel) are scaled by the config's `currency_factor`


Mixed IAM-REMIND docu, also suggest breaking down

- **Costs and technical parameters** can be sourced by the IAM, by the PyPSA cost table or directly set to a user-set value. This is governed by a `yaml` mapping. The regional discount rate can be read directly from supported IAMs via `Coupler.build_discount_rates(year)` and is added to the techno-economics table used by the coupled PyPSA. - **Currency conversion and discount rates** currencies are converted from the IAM basecurrency (e.g. USD2017 for REMIND3.6) to the PyPSA currency (e.g. EUR2015 for PyPSA-Eur) by the `currency_factor` parameter (1.0 is no op). This factor is set in XXXXX[not the tech mapping?] and is *not intended* for inflation correction or other.

irr-github · 2026-08-03T09:49:54Z

@@ -21,9 +21,12 @@ A few things worth knowing about all four:
  (`remind_symbols_gdx.yaml` / `remind_symbols_mif.yaml` for the REMIND backends) and applied


mixed REMIND-IAM docu

maybe we should find a better name for symbols?

irr-github · 2026-08-03T10:23:02Z

@@ -27,18 +22,63 @@ def annotate_cost_rows(
    costs: pd.DataFrame,


is this an overwrite? an addition? annotate sounds a lot like add a comment

irr-github · 2026-08-03T10:25:53Z

+
+
+# TODO: Implement more generic deflator for different currency years
+def apply_currency_factor(


maybe add a warning that it does not discriminate btween pypsa and IAM entries? Ie only IAM entries should be passed on the IAM->PyPSA

irr-github · 2026-08-03T10:28:07Z

+    """Label and reduce already-converted IAM demand to one row per (year, region, sector).
+
+    Assumes ``raw`` is already in the target unit (conversion happens at the ``load_frame``
+    seam); sums rows sharing a key as a guard against an unexpected extra source dimension.


this doesn't make much semantic sense to me!

If I read the changes right, convert_loads no longer converts (it just selects) and load now converts?

If you want to apply this, it needs a redesign (e.g. remove transform.loads entirely) and also docu updates. I am rather against it

irr-github · 2026-08-03T10:28:55Z


 import pandas as pd

-# Centralized in iampypsa.units (GDX backend reports per tonne carbon; PyPSA wants per tonne CO2).


exact same comment about conversions and transforms as in loads.py

irr-github · 2026-08-03T10:30:59Z

-
-    tech = caps[tech_col].astype(str)
-    is_btin_present = ((tech == "btin") & (caps[value_col] > 0)).any()
-    is_stor = tech.isin(battery_scaling)


can you explain the logic of moving this to Coupler (not against it just unclear what the idea is)

aodenweller added 6 commits July 24, 2026 13:24

Continue review

6322f44

- Updated documentation in architecture and technology mapping - Remove `unit_factor` in `build_regional_demand` - Remove `eta_exponents` in `convert_investment_to_input_capacity_basis` - Remove any mention of "tidy frames" throughout - Update docstrings

Refactor unit handling and standardize currency units

16b69f1

Update release workflow URL, add test workflow, and refactor package …

eb568cc

…data handling

Remove deprecated conftest.py

7c529c7

aodenweller requested a review from irr-github July 27, 2026 14:08

aodenweller mentioned this pull request Jul 27, 2026

Rewrite package #13

Open

Remove deprecated functions

5b3eb8d

irr-github reviewed Aug 3, 2026

View reviewed changes

Navigation Menu

Uh oh!

Review and refactor before merging develop with main - #15

Review and refactor before merging develop with main#15
aodenweller wants to merge 7 commits into
pik-piam:developfrom
aodenweller:develop_review

aodenweller commented Jul 27, 2026 •

edited

Loading

Uh oh!

irr-github left a comment

Uh oh!

irr-github Aug 3, 2026

Uh oh!

irr-github Aug 3, 2026

Uh oh!

irr-github Aug 3, 2026

Uh oh!

irr-github Aug 3, 2026

Uh oh!

irr-github Aug 3, 2026

Uh oh!

irr-github Aug 3, 2026

Uh oh!

irr-github Aug 3, 2026

Uh oh!

irr-github Aug 3, 2026

Uh oh!

irr-github Aug 3, 2026

Uh oh!

irr-github Aug 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		@@ -21,9 +21,12 @@ A few things worth knowing about all four:
		(`remind_symbols_gdx.yaml` / `remind_symbols_mif.yaml` for the REMIND backends) and applied

		@@ -27,18 +22,63 @@ def annotate_cost_rows(
		costs: pd.DataFrame,



		# TODO: Implement more generic deflator for different currency years
		def apply_currency_factor(


		import pandas as pd

		# Centralized in iampypsa.units (GDX backend reports per tonne carbon; PyPSA wants per tonne CO2).

Uh oh!

Conversation

aodenweller commented Jul 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

To be decided

Uh oh!

irr-github left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

aodenweller commented Jul 27, 2026 •

edited

Loading