added switch to turn off yield calib

[DavidhoPIK]

🐦 Description of this PR 🐦

Add a switch to turn off the use of yield calibration factors

🔧 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)

[pfuehrlich-pik]

I see this only a draft PR so you might have had that in mind anyway, but a short note in the changelog would be appreciated :)

[DavidhoPIK]

Thank you Pascal. I added an entry in the changelog. One question is left for me: Do I need to change the scenario_config.csv ?

[pfuehrlich-pik]

Thank you Pascal. I added an entry in the changelog. One question is left for me: Do I need to change the scenario_config.csv ?

I'm not sure, better ask someone else, I'm not using the scenario_config.csv
My feeling would be yes, similar to the change you made to the fsec scenario config

[DavidhoPIK]

Thank you Pascal. I added an entry in the changelog. One question is left for me: Do I need to change the scenario_config.csv ?

I'm not sure, better ask someone else, I'm not using the scenario_config.csv My feeling would be yes, similar to the change you made to the fsec scenario config

I made the change in the FSEC scenario because it explicitly uses the penalty_apr_22 crop realization in the scenario file. When using this realization it is recommended to still have yield calibration on.
I just talked with @FelicitasBeier : The other scenario files do not specify the use of this crop realization. This is why we think in theses scenarios we stick to the default and thus not change the scenario files.

[DavidhoPIK]

Another question: How can I change the label? (Low risk, medium risk, high risk)? @FelicitasBeier and I think it is a Low risk PR.

[DavidhoPIK]

One more question: Is think that I do not need to change magpie4 R library for this change, is that correct? @pfuehrlich-pik

[DavidhoPIK]

Another question: How can I change the label? (Low risk, medium risk, high risk)? @FelicitasBeier and I think it is a Low risk PR.

Actually when reading the description of the different risk levels, I'm not sure anymore if this is really low risk. It is a change in the default settings. @FelicitasBeier @pfuehrlich-pik what do you think?

[DavidhoPIK]

Another question: How can I change the label? (Low risk, medium risk, high risk)? @FelicitasBeier and I think it is a Low risk PR.

Actually when reading the description of the different risk levels, I'm not sure anymore if this is really low risk. It is a change in the default settings. @FelicitasBeier @pfuehrlich-pik what do you think?

Another question: How can I change the label? (Low risk, medium risk, high risk)? @FelicitasBeier and I think it is a Low risk PR.

Actually when reading the description of the different risk levels, I'm not sure anymore if this is really low risk. It is a change in the default settings. @FelicitasBeier @pfuehrlich-pik what do you think?

I did a default test run and confirmed that no calibration is performed with default settings now. (Also in the case of a the f14_yld_[..].csv in the input directory) I also started test runs with Rscript start.R --> "test runs" for the case that we decide the risk is higher than low. (Not sure how long those take)

[DavidhoPIK]

Unfortunately all test runs except "weeklyTests_singleTimeStep" failed. The default run worked as said before. I'm now trying to figure out why the test runs failed. In case you want to have a look:
/home/davidho/MAgPIEG/MAgPIEnoYCalibPR/magpie-EU/output

[DavidhoPIK]

Unfortunately all test runs except "weeklyTests_singleTimeStep" failed. The default run worked as said before. I'm now trying to figure out why the test runs failed. In case you want to have a look: /home/davidho/MAgPIEG/MAgPIEnoYCalibPR/magpie-EU/output

The test failed because my home directory was full. Tests are rerunning now here p/projects/landuse/users/beier/yieldCalibSwitch/magpie-EU

[DavidhoPIK]

As required for a high risk merge request I did runs to compare

1. the new branch with default settings (name in shiny: default_settings)
2. the new branch with s14_use_yield_calib switched on (name in shiny: yield_calib_switched_on)
   with
3. the target development branch (default)
   Here are some relevant comparisons:

intended/expected changes

crop

pasture

The new branch with s14_use_yield_calib switched on has identical results as the target branch as expected. This is why the red line is always below the blue line and not visible (I double checked this).

additional comparisons

[DavidhoPIK]

In terms of test runs i get this warning from the tests:
** Warning ** Some variables are considered fixed because their
initial bounds are very close within a relative
distance of Tol_Fixed = 4.0E-10
I read this more as a message from gams than a warning and find this not worrying.
Edit: I just checked: The same warning also occurs in the current development target branch.
Other than that all tests ran smoothly without warnings now.

[DavidhoPIK]

Since all points are checked, from my side the PR is ready to review.

[FelicitasBeier]

Thank you Pascal. I added an entry in the changelog. One question is left for me: Do I need to change the scenario_config.csv ?

I'm not sure, better ask someone else, I'm not using the scenario_config.csv My feeling would be yes, similar to the change you made to the fsec scenario config

Since we want the yield calibration to be off by default, this should also apply for the general scenario config. The FSEC one needed to be adjusted because we use the penalty_apr22 crop realization there.

[FelicitasBeier]

In terms of test runs i get this warning from the tests: ** Warning ** Some variables are considered fixed because their initial bounds are very close within a relative distance of Tol_Fixed = 4.0E-10 I read this more as a message from gams than a warning and find this not worrying. Edit: I just checked: The same warning also occurs in the current development target branch. Other than that all tests ran smoothly without warnings now.

Interesting that it also occurs in the current develop. This means it doesn't prevent this one from being merged.
But just to be sure:
maybe @tscheypidi @mishkos @flohump could you comment whether this is something we should be worried about?

[FelicitasBeier]

Looks good to me. Thanks!

[tscheypidi]

In terms of test runs i get this warning from the tests: ** Warning ** Some variables are considered fixed because their initial bounds are very close within a relative distance of Tol_Fixed = 4.0E-10 I read this more as a message from gams than a warning and find this not worrying. Edit: I just checked: The same warning also occurs in the current development target branch. Other than that all tests ran smoothly without warnings now.

Interesting that it also occurs in the current develop. This means it doesn't prevent this one from being merged. But just to be sure: maybe @tscheypidi @mishkos @flohump could you comment whether this is something we should be worried about?

I am not worried about that. I think it is conceptually and technically acceptable if some variables have no room to move and are therefor being fixed by GAMS.