Skip to content

Remove redundant m_boundfix calls, fix to up instead of l - #943

Merged
flohump merged 4 commits into
magpiemodel:developfrom
flohump:boundfix-cleanup
Oct 8, 2026
Merged

flohump merged 4 commits into
magpiemodel:developfrom
flohump:boundfix-cleanup

Conversation

@flohump

@flohump flohump commented Oct 4, 2026 •

Copy link
Copy Markdown
Contributor

🐦 Description of this PR 🐦

Cleans up the m_boundfix calls. Results are unchanged.

  • Simplifies the macro to m_boundfix(x,arg), always fixing to up with sensitivity 1e-6. All calls used these values except three sites with l (29 fallow, 35 secdforest, 35 other land), where .l equalled .up whenever the macro fired.
  • Removes the call for 29_cropland tree cover (the variable is fixed one line above).
  • The calls are needed: with the macro switched off, the run aborts in 2005 (lower bound > upper bound on v35_secdforest, rounding of order 1e-17), and up to ~1500 near-fixed v35_secdforest entries per step are left to CONOPT.
  • Test: SSP2 NPi and PkBudg650, develop (47249fd) vs this branch: identical objective in all 18 solves, gdxdiff of fulldata.gdx differs only in c_title.

🔧 Checklist for PR creator 🔧

If a point is not applicable, check the checkbox anyway and write "non-applicable" next to the checkbox.

  • Label pull request from the label list.

    • Low risk: Simple bugfixes (missing files, updated documentation, typos) or changes in start or output scripts
    • Medium risk: Uncritical changes in the model core (e.g. moderate modifications in non-default realizations)
    • High risk: Critical changes in model core or default settings (e.g. changing a model default or adjusting a core mechanic in the model)
  • Self-review own code

    • No hard coded numbers and cluster/country/region names.
    • The new code doesn't contain declared but unused parameters or variables.
    • magpie4 R library has been updated accordingly and backwards compatible where necessary.
    • scenario_config.csv has been updated accordingly (important if default.cfg has been updated)
  • Document changes

    • Add changes to CHANGELOG.md
    • Where relevant, put In-code documentation comments
    • Properly address updates in interfaces in the module documentations
    • run goxygen::goxygen() and verify the modified code is properly documented
  • Perform test runs

    • Low risk:
      • Run a compilation check via Rscript start.R --> "compilation check"
    • Medium risk:
      • Run default run via Rscript start.R --> "default"
      • Check logs for errors/warnings
      • Fill in performance section below
    • High risk:
      • Run test runs via Rscript start.R --> "test runs"
      • Check logs for errors/warnings
      • Default run from the PR target branch for comparison
      • Provide relevant comparison plots (land-use, emissions, food prices, land-use intensity,...)
      • Fill in performance section below
  • Reporting produces no errors and no new warnings

  • Get two approving reviews (at least one from RSE)

📉 Performance 📈

  • This PR's default : 35 mins
  • For comparison: runtimes of weekly test

🚨 Checklist for reviewer 🚨

  • PR is labeled correctly
  • Code changes look reasonable
    • No hard coded numbers and cluster/country/region names.
    • No unnecessary increase in module interfaces
    • model behavior/performance is satisfactory.
  • Changes are properly documented
    • CHANGELOG is updated correctly
    • Updates in interfaces have been properly addressed in the module documentations
    • In-code documentation looks appropriate

The calls in 10_land (runs before modules 31-35 set the vm_land bounds) and 29_cropland tree cover (variable fixed one line above) never change anything. At the remaining l sites, .l equals .up when the macro fires, so up gives identical results without depending on statement order.
Comment thread CHANGELOG.md Outdated
@flohump flohump added High risk Higher risk bugfix AI An AI model was used for this modification labels Oct 5, 2026
@flohump
flohump marked this pull request as ready for review October 5, 2026 09:53

@tscheypidi tscheypidi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for tackling this. Two requests:

  • I think the boundfix in module 10 must not be removed as you cannot assume that modules 31-35 will change the bounds. Is there any harm of having it?

  • I checked the different occurrences of the boundfix and with "l" now not being recommended I think all of them set the bound to up and the sensitiviy to 1e-6. For simplification and reduction of user error I would suggest to change the macro and remove the option to set sufx and sens, Just $macro m_boundfix(x,arg) x.fx arg$(x.up arg-x.lo arg<1e-6) = x.up arg; instead

The macro always fixes to up with sensitivity 1e-6, the values used at all call sites. Keep the 10_land call, since not every realization of 29-35 resets the vm_land bounds (review by tscheypidi).
@flohump

flohump commented Oct 8, 2026

Copy link
Copy Markdown
Contributor Author

@tscheypidi Thanks. I adressed both of your points. Please check the code once more.

@flohump
flohump requested a review from tscheypidi October 8, 2026 08:30

@tscheypidi tscheypidi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

looks good, thanks

@flohump
flohump merged commit 66aa2bc into magpiemodel:develop Oct 8, 2026
2 checks passed
@flohump
flohump deleted the boundfix-cleanup branch October 8, 2026 09:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI An AI model was used for this modification bugfix High risk Higher risk

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants