Skip to content

Clean up core declarations - #2189

Merged
fbenke-pik merged 37 commits into
remindmodel:developfrom
fbenke-pik:refactor
Sep 23, 2025
Merged

Clean up core declarations#2189
fbenke-pik merged 37 commits into
remindmodel:developfrom
fbenke-pik:refactor

Conversation

@fbenke-pik

@fbenke-pik fbenke-pik commented Aug 25, 2025

Copy link
Copy Markdown
Contributor

Purpose of this PR

  • Move declarations from core to more fitting modules
  • Remove no longer used parameters and equations

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?):

@fbenke-pik fbenke-pik changed the title remove vm_dummyBudget Clean up core declarations Aug 29, 2025
@fbenke-pik
fbenke-pik marked this pull request as ready for review September 12, 2025 12:37

@fschreyer fschreyer 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.

Great, thanks! Could you do an NPi and a PkBudg650 run before and after your changes and check in the compare scenario PDF whether there are differences? I had differences in this PR (#2142) which presumably related to the order in which variables / parameters are declared in GAMS. I hope this is not the case this time, but would be worth to check.

Comment thread core/declarations.gms
Comment thread modules/30_biomass/magpie_40/equations.gms
@robertpietzcker

robertpietzcker commented Sep 15, 2025

Copy link
Copy Markdown
Contributor

Hi @fbenke-pik @LaviniaBaumstark

I just saw this now, I have to admit I am a bit surprised. Concretely, I wanted to ask: is there a general change of rules/coding etiquette for Xm_XXX-type declarations, and if yes, what were the reasons for this change?

I had always understood the rule to be that we only declare parameters and variables inside modules that are only used in this model and thus have the module number in the name, while all interface parameters should be declared in code. I have to admit I found this rule quite clear and easy to apply, and it was easy to check the declaration because you know from the name either in which module to check or to go to core (if it has Xm_XXX). Some interface variables may be relevant to several modules, which means it may be unclear where to put them; also, interface variable declarations will need to be repeated in each model realization.

I am sure I will be able to adapt :-) , I am just wondering about the "why" of this refactoring, and if there is a clear rule which declarations go into core, and which into the modules?

@LaviniaBaumstark

Copy link
Copy Markdown
Member

Hi,
This is no new rule. You are right, that teh name of a varible/paramter indicates, if it is an interface or only used in one moduel or in core. Interface valiables (vm_, pm_) might be interfaces between the core and a module or only between two (or more) moduels. So if a modle provides a variable/parameter, which is needed in another module but not used in the core, we do not want to get another unncessary interface to the core. We try to declare all varialbesTparameter in the moduels, where they are calculated.

@fschreyer

fschreyer commented Sep 16, 2025

Copy link
Copy Markdown
Contributor

I had always understood the rule to be that we only declare parameters and variables inside modules that are only used in this model and thus have the module number in the name, while all interface parameters should be declared in code.

Hi Robert,

a few thoughts on why I think declaring all interface variables in the core is really not the way forward and we should stop this practice:

  1. Modularization of content: parameters and variables that conceptually belong together should be in one place of the code. Ideally, this means to approach a situation where newcomers / externals can just read the code in their module and understand to a large what is happening or what they are responsible for. They will know the inputs / outputs of their module (the interface variables) and the module variables that link them.

  2. Declaring interfaces in the core led to a very (!) long list of declarations without much order that often let people overlook whether the a certain variables / parameter is already declared or a certain concepts already exists. It moreover encourages a practice of "if I don't know where it belongs, I put it in the core" and thereby prevents people from structurally thinking about how the code they write should reflect the conceptual containers that exist in the model and in our own methodological thinking.

  3. Keeping this practice would never allow to dissolve / largely reduce the core. I know this may seems like a far-fetched goal but may be really worth it.

  4. Moving declarations to modules is ideally just one step. Moving parts of other core files to modules is even more important.

  5. I think the rule that Lavinia mentioned is still clear: A parameter or variable should be calculated / read-in in the module it is (mostly) used. And a parameter / variable should be declared where it is calculated / read-in. Now, variables are used in constraints so they are not "directly" calculated but depend on various constraints. There may be some difficult cases but in those ambiguous cases I think it may be generally useful to think about what "defines" a variables and where is a variable "used" (e.g. I would argue that emissions variables are "defined" by the emissions equations but are "used" in the tax equations). Generally, in my view fostering more conscious thinking about inputs and outputs into a module would greatly help the REMIND code.

We can also follow-up on the discussion here: https://github.com/remindmodel/development_issues/issues/618

Comment thread modules/46_carbonpriceRegi/none/declarations.gms Outdated
Comment thread modules/46_carbonpriceRegi/none/declarations.gms Outdated
Comment thread modules/46_carbonpriceRegi/netZero/declarations.gms Outdated
Comment thread modules/46_carbonpriceRegi/netZero/declarations.gms Outdated
Comment thread modules/46_carbonpriceRegi/NDC/declarations.gms Outdated
Comment thread modules/46_carbonpriceRegi/NDC/declarations.gms Outdated
@robertpietzcker

Copy link
Copy Markdown
Contributor

@LaviniaBaumstark @fschreyer thanks for sharing your thoughts!

Comment thread modules/46_carbonpriceRegi/NDC/declarations.gms Outdated
@fbenke-pik

Copy link
Copy Markdown
Contributor Author

Great, thanks! Could you do an NPi and a PkBudg650 run before and after your changes and check in the compare scenario PDF whether there are differences? I had differences in this PR (#2142) which presumably related to the order in which variables / parameters are declared in GAMS. I hope this is not the case this time, but would be worth to check.

The requested comparison can be found here /p/tmp/benke/model/remind_refactor/compScen-2025-09-22_14.55.07-H12.pdf

@fschreyer

Copy link
Copy Markdown
Contributor

I had a look at the comparison PDF and it mostly looks fine. The only point where there are slight differences is transport energy service demand in the US. It appears that with the refactoring of the REMIND code EDGE-T behaves slightly different in this region. I also observed that in my refactoring #2142. It might be that EDGE-T is not stable there against small changes of input GDX or order of declarations?

I think in this case this should not stall merging this. Could somebody of the transport team have a look and say whether that's an issue. @robertpietzcker

@robertpietzcker

Copy link
Copy Markdown
Contributor

@fschreyer I don't think this is a result of the refactoring (sometimes the refactored runs are much higher, sometimes the non-refactored runs are much higher), but I guess it is rather a fundamental problem of non-convergence between REMIND and EDGE-T in the US: in some of the runs, liquids prices spike high, with low aviation as a result, while in the others the opposite occurs. I would guess this flip-flops between iterations.

image image

I have no idea where it comes from, and I hope this was just a number of unlucky runs - the latest validation runs don't show it.

The transport team has on its radar to implement a convergence test between REMIND and EDGE-T, but it will be some time before this happens
(including @johannah-pik @jmuessel @ahagen-pik here - not for doing anything about it, just for awareness about this problem)

@fbenke-pik

Copy link
Copy Markdown
Contributor Author

Thanks for your input, Robert and Felix. I read your comments as an ok to merge.

@fbenke-pik
fbenke-pik merged commit 729adc3 into remindmodel:develop Sep 23, 2025
2 checks passed
@fbenke-pik
fbenke-pik deleted the refactor branch September 23, 2025 14:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants