Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added restart points and automatic restart support #512

Merged
merged 10 commits into from
Jan 23, 2023

Conversation

tscheypidi
Copy link
Member

@tscheypidi tscheypidi commented Jan 20, 2023

馃惁 Description of this PR 馃惁

After this update the model will create restart files after each time steps and use these files if the model is stopped at some point and restarted (previously a restarted model would start again from the first time step)

In addition:

  • fixed a merge mistake in CHANGELOG.md
  • added standby and priority qos-setups with 24h limit (relevant for runs close to a cluster maintenance window)

馃敡 Checklist for PR creator 馃敡

  • 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 test runs via Rscript start.R --> "test runs"
      • Check logs for errors/warnings
    • 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,...)

馃搲 Performance changes 馃搱

  • Current develop branch default : ** mins
  • This PR's default : ** mins

馃毃 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
  • content review done (at least 1)
  • RSE review done (at least 1)

CHANGELOG.md Outdated
- **59_som** added new **cellpool_jan23** realization with updated 2019 IPCC guidelines values
- **config** added setting cfg$keep_restarts which controls whether restart files should be kept after a run finished
- **scripts** added restart points after each time step from which the model can now be restarted if the simulation aborts at some point
- **scripts** added start script which starts an empty model just regenerating a previous run
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- **scripts** added start script which starts an empty model just regenerating a previous run

This is already in the changelog under 4.6.3

CHANGELOG.md Outdated
Comment on lines 15 to 16
- **31_past** added additional limitation (single climate scenario input) for **grasslands_apr22**
- **59_som** added new **cellpool_jan23** realization with updated 2019 IPCC guidelines values
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- **31_past** added additional limitation (single climate scenario input) for **grasslands_apr22**
- **59_som** added new **cellpool_jan23** realization with updated 2019 IPCC guidelines values

This is already in the changelog under 4.6.3

config/default.cfg Show resolved Hide resolved
@@ -147,24 +149,24 @@ $title magpie
*##################### R SECTION START (VERSION INFO) ##########################
*
* Used data set: rev4.79_h12_magpie.tgz
* md5sum: 4f3f5fd72716fe371d646c69c30e6fd3
* Repository: /p/projects/rd3mod/inputdata/output
* md5sum: NA
Copy link
Contributor

Choose a reason for hiding this comment

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

Unrelated to this PR, but why is the md5sum NA when the data comes from our public repo?

Copy link
Member Author

Choose a reason for hiding this comment

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

idk

@flohump flohump added High risk Higher risk enhancement New feature or request Minor Smaller modifications and removed Low risk Low risk labels Jan 23, 2023
Copy link
Contributor

@flohump flohump left a comment

Choose a reason for hiding this comment

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

Thanks for adding this very useful feature!
The code looks fine to me.
However, there are changes in the model core.
Therefore, this qualifies as high risk PR (I changed this in the PR).
Please make test runs to check if the model results are unchanged.

@tscheypidi
Copy link
Member Author

Our labeling policy is here a bit vague, but I would still see this PR as a low risk one as there are no "critical changes" in the code: My PR does not contain any changes which would affect the way things are being calculated in the model. All changes are pure technical nature and I tested that they only get active in instances where they should.

However, to be completely sure I also did now the set of test runs and as expected they are 1:1 identical to previous test runs.

Copy link
Contributor

@flohump flohump left a comment

Choose a reason for hiding this comment

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

I agree, our PR policy is a bit vague here.
Thanks for doing the test runs anyways.

@tscheypidi tscheypidi merged commit c0d417b into magpiemodel:develop Jan 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request High risk Higher risk Minor Smaller modifications
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants