Skip to content

improve and restructure core/declarations: add units where missing, improve descriptions, restructure along themes, add suggestions to move declarations to modules - #2142

Merged
fschreyer merged 10 commits into
remindmodel:developfrom
fschreyer:refac
Aug 19, 2025

Conversation

@fschreyer

@fschreyer fschreyer commented Jul 1, 2025

Copy link
Copy Markdown
Contributor

Purpose of this PR

This restructures core/declarations and improves documentation. This is a step towards enhancing the modularization of REMIND by moving parts out the core to specific modules (see https://github.com/remindmodel/development_issues/issues/618). It is a pure refactoring and should not affect the results.

This change structures the declarations in the core into 5 categories and adds suggestions which declarations could be moved out to other modules (possibly in a later step to not overload this PR). These are the 5 categories:

  1. Macro-Economy
  2. Emissions
  3. Energy System
  4. Carbon Management
  5. Other (general parameters / conversion factors used throughout the model)

I improved the documentation to the best of my knowledge but ofc I may overlook things. Note that I also used AI (github co-pilot) for parts of the tasks of moving code and reformating it. So, it would be important that for each section some people check whether they spot any issues or would suggest different categorization or documentation. Also check whether declarations that are flagged as "can be removed" can actually be removed (or had a purpose which got lost by accident or has been overlooked).

I'd appreciate feedback from the different people on the energy system (@lecfab, @Renato-Rodrigues), emissions (@Renato-Rodrigues, @amerfort, @gabriel-abrahao), Macro and Other category (@LaviniaBaumstark).

Update:

As the restructuring caused slight perturbations in the solver route that led to different peak budget year solutions, another convergence criterion was added for peak budget runs that checks that either

  • cm_peakBudgYr = time step with maximum cumulative CO2 emissions
    OR
  • difference in cumulative emissions between both time steps is below sm_peakbudget_diff_tolerance

see https://github.com/remindmodel/development_issues/issues/623

Next steps:
(to be followed up in subsequent PRs)

  • remove deprecated / unused declarations
  • move declarations to modules that were flagged as more fitting / mainly used in another module

Here is a comparison with the version before and after the refactoring. NPi and PkBudg650 runs tested are practically identical.

/p/tmp/schreyer/Modeling/remind/refac/compScen-Refac_Before_After-2025-07-22_09.43.55-H12.pdf

Type of change

Indicate the items relevant for your PR by replacing ◻️ with ☑️.
Do not delete any lines. This makes it easier to understand which areas are affected by your changes and which are not.

Parts concerned

  • ☑️ GAMS Code
  • ◻️ R-scripts
  • ☑️ Documentation (GAMS incode documentation, comments, tutorials)
  • ◻️ Input data / CES parameters
  • ◻️ Tests, CI/CD (continuous integration/deployment)
  • ◻️ Configuration (switches in main.gms, default.cfg, and scenario_config*.csv files)
  • ◻️ Other (please give a description)

Impact

  • ◻️ Bug fix
  • ☑️ Refactoring
  • ◻️ New feature
  • ◻️ Change of parameter values or input data (including CES parameters)
  • ◻️ Minor change (default scenarios show only small differences)
  • ◻️ Fundamental change of results of default scenarios

Checklist

Do not delete any line. Leave unfinished elements unchecked so others know how far along you are.
In the end all checkboxes must be ticked before you can merge
.

  • I executed the automated model tests (make test) after my final commit and all tests pass (FAIL 0)
  • I adjusted the reporting in remind2 if and where it was needed
  • I adjusted the madrat packages (mrremind and other packages involved) for input data generation if and where it was needed
  • My code follows the coding etiquette
  • I explained my changes within the PR, particularly in hard-to-understand areas
  • I checked that the in-code documentation is up-to-date
  • I adjusted forbiddenColumnNames in readCheckScenarioConfig.R in case the PR leads to deprecated switches
  • I updated the CHANGELOG.md correctly (added, changed, fixed, removed, input data/calibration)

Further information (optional)

  • Runs with these changes are here:
  • Comparison of results (what changes by this PR?):

@fschreyer fschreyer added documentation Improvements or additions to documentation code cleaning Code that could/should be cleaned up labels Jul 1, 2025
Comment thread core/declarations.gms
Comment thread core/declarations.gms Outdated
Comment thread core/declarations.gms Outdated
Comment thread core/declarations.gms Outdated
Comment thread core/declarations.gms
Comment thread core/declarations.gms Outdated
Comment thread core/declarations.gms Outdated
Comment thread core/declarations.gms

@lecfab lecfab left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this vast undertaking Felix. The structure will make declarations much clearer and allow us to put chunks in modules when relevant.

Comment thread core/declarations.gms Outdated

*** co2 price mark-up per region on top of pm_taxCO2eq calculated in 46_carbonpriceRegi module
pm_taxCO2eqRegi(tall,all_regi) "Additional regional CO2 tax path calulated in in 46_carbonpriceRegi module to reach regional emissions targets [T$/GtC]. To get $/tCO2, multiply with 272 [T$/GtC]"
pm_taxCO2eqSum(tall,all_regi) "sum of pm_taxCO2eq, pm_taxCO2eqRegi, pm_taxCO2eqSCC [T$/GtC]. To get $/tCO2, multiply with 272 [T$/GtC]"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pattern of comment comes back many times:

"explanation [T$/GtC]. To get $ /tCO2, multiply with 272 [T$/GtC]"

I would remove the second unit bracket, and eventually replace by

"explanation [T$/GtC]. To get $/tCO2, multiply with s_xxx ≈ 272"

With s_xxx a conversion factor, like those exist:

s_MtCO2_2_GtC "conversion factor from MtCO2 to native REMIND emission unit GtC" /2.727e-04/
sm_DptCO2_2_TDpGtC "Conversion multiplier to go from $/tCO2 to T$/GtC: 44/12/1000"     /0.00366667/

Comment thread core/declarations.gms
sm_tgn_2_pgc = (44/28) * s_gwpN2O * (12/44) * 0.001;
sm_tgch4_2_pgc = s_gwpCH4 * (12/44) * 0.001;

*** carbon intensities of coal, oil, and gas
*** carbon intensities of coal, oil, and gas (move to core datainput?)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely, but it could maybe even go to 21_tax as it is mostly involved there.

Comment thread core/declarations.gms

pm_cintraw(all_enty) "carbon intensity of fossils [GtC per TWa]"
*** parameters used for 2005 capacity calibration (move to module 5 intialCap?)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would agree, but it looks like pm_EN_demand_from_initialcap2 has a role in 24_trade, and pm_vintage_in in core

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally, all of this code should be moved to initialCap then. Content-wise, it all has to do with getting the 2005 state right.

Comment thread core/declarations.gms Outdated
o_avgAdjCostInv(ttot,all_regi,all_te) "average adjustment cost calculated in postsolve for diagnostics"
o_avgAdjCost_2_InvCost_ratioPc(ttot,all_regi,all_te) "ratio in % of average adj cost compared to direct inv costs"
*** energy service layer (only relevant for transport, move to transport module?)
pm_fe2es(tall,all_regi,all_teEs) "Conversion factor from final energies to transport energy services [Tpkm/TWa, Ttkm/Twa]"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TWa

Comment thread core/declarations.gms
$ifthen.minMaxSeFeSectorShareDev "%cm_seFeSectorShareDevMethod%" == "minMaxAvrgShare"
q_minMaxPenSeFeSectorShareDev(ttot,all_regi,all_enty,all_enty,emi_sectors,all_emiMkt) "min-max penalty balance for secondary energy share deviation in sectors"
$endif.minMaxSeFeSectorShareDev
p_adj_coeff_glob(all_te) "coefficient for adjustment costs - global scale [unitless]"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would it make sense to put together all the adjustment cost declarations?
Like p_adj_coeff_Orig, p_adj_seed_te_Orig, p_adj_coeff_glob, v_adjFactor...
(also if you understand them, documenting their differences would be very helpful 🙏 )

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this was done above. I think this specific parameter can be removed. That is why I sorted it to the bottom.

Comment thread core/declarations.gms
pm_conv_TWa_EJ "conversion from TWa to EJ" /31.536/,
sm_c_2_co2 "conversion from c to co2" /3.666666666667/,
*** conversion factors of time units
*** ------------Unit Conversion Factors---------------

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that they are all reorganised, would be nice to make the conversion numbers consistent: so far they always have different numbers of decimals, different roundings etc..

Examples:

  • s_zj_2_twa /31.7098/, but sm_EJ_2_TWa /31.71e-03/ (adding more digits is maybe unnecessary, as we already use the approximation 1a=365j)
  • sm_DptCO2_2_TDpGtC /0.00366667/, but sm_c_2_co2 /3.666666666667/,
  • etc

As it would change results a tiny bit, it could be annoying to put it in a PR where you want to test that results stay unchanged. But in this one it could make sense as the reordering has no impact?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. Perhaps lets do that in another PR.

Comment thread core/declarations.gms
o_carbon_underground(ttot,all_regi,all_enty) "output parameter"
o_carbon_reemitted(ttot,all_regi,all_enty) "output parameter"

o_emi_conv(all_enty) "output parameter" / co2 3666.6666666666666666666666666667, ch4 28, n2o 416.4286, so2 1, bc 1, oc 1 /

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see my comment on units elsewhere: if we use that many decimals for co2, this should also appear in the conversion factors s_...

@fschreyer

Copy link
Copy Markdown
Contributor Author

Here some of the comparison plots:

image image image

@fschreyer
fschreyer marked this pull request as ready for review July 23, 2025 12:41
@Renato-Rodrigues

Copy link
Copy Markdown
Member

I am agree with the merge as long as a decision is reached in https://github.com/remindmodel/development_issues/issues/623

@fschreyer

fschreyer commented Aug 18, 2025

Copy link
Copy Markdown
Contributor Author

@LaviniaBaumstark I would like to merge this now as it gets increasingly annoying to keep this branch up-to-date with develop. Any objections?

@lecfab

lecfab commented Aug 18, 2025

Copy link
Copy Markdown
Contributor

Advertising here for another restructuring that i'd like to merge soon but don't know who should review it:

@fschreyer
fschreyer merged commit 27449d6 into remindmodel:develop Aug 19, 2025
2 checks passed
@fschreyer fschreyer mentioned this pull request Sep 15, 2025
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

code cleaning Code that could/should be cleaned up documentation Improvements or additions to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants