bugfixes ac_est #624

flohump · 2024-01-15T09:50:37Z

🐦 Description of this PR 🐦

bugfixes for ac_est in 35_natveg and 32_forestry

🔧 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 : 24 mins
This PR's default : 24 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)

FelicitasBeier · 2024-01-16T09:28:01Z

modules/35_natveg/dynamic_feb21/equations.gms

              + v35_primforest_reduction(j2));

- q35_other_expansion(j2,ac_est) ..
-  v35_other_expansion(j2,ac_est) =e=
-    v35_other(j2,ac_est) - pc35_other(j2,ac_est);


To me it's not clear, why it is no longer necessary to subtract pc35_other here.
Now expansion is just equal to the stock?

Is the pc35_other parameter still used somewhere or not necessary anymore?

And I have the same question for: q32_land_expansion(j2,type32) and q35_secdforest_expansion(j2,ac_est)

pc35_other(j2,ac_est) is always zero because other land added in the previous time step has been shifted to higher age-classes in-between the iterations. The same holds true for q32_land_expansion and q35_secdforest_expansion. pc35_other(j2,ac_sub) is the other part of the full ac set, which in the optimization can only decrease.

In that case, should the name of the variables and equations change to something pointing towards "aggregated forestry land pools"? The expansion part is confusing (or is the naming related to specific jargon used in the forestry field?). I thought pc32_land and pc35_other were related to the information from the previous step.

To which variables and equations are you referring regarding the naming?
Expansion refers to land gross land expansion and can be directly linked to the ac_est subset. Reduction refers to gross land reduction and can be direclty linked to the ac_sub subset.
Yes, pc32_land and pc35_other hold information from the previous time step, but in the case of forestry and natveg the shifting for regrowth happens between the time steps. This has to be accounted for.

I refer to q32_land_expansion(j2,type32), q35_secdforest_expansion(j2,ac_est), v35_other_expansion(j2,ac_est), and v32_land_expansion(j2,ac_est). So, is the difference between the previous and current steps (expansion) handled with the ac_sub dynamic set?

No. In the optimization, expansion can only happen in v35_other(j2,ac_est) and reduction can only happen in v35_other(j2,ac_sub). ac_est and ac_sub are dynamic sets, depeding on the time step lengths. Because of the age-class shifting ac_est will always be zero in p35_other(j2,ac_est). Therefore, for expansion only v35_other(j2,ac_est) is needed. For the reduction, both v35_other(j2,ac_sub) and p35_other(j2,ac_sub) are needed. The areas added in the previous time step are included in p35_other(j2,ac_sub). Same holds true for v32_land and v35_secdforest.

FelicitasBeier

The emissions and tau behavior in CAZ is quite extreme. Where does it come from? Was it like this before and we just return to previous behavior or what explains the change?

flohump · 2024-01-16T09:38:59Z

The emissions and tau behavior in CAZ is quite extreme. Where does it come from? Was it like this before and we just return to previous behavior or what explains the change?

This is a known issue. We have seen this in recent test runs. There is no content-wise connection to the changes of this PR. For full transparency, I added the figures for CAZ and Global.

FelicitasBeier · 2024-01-16T09:44:03Z

The emissions and tau behavior in CAZ is quite extreme. Where does it come from? Was it like this before and we just return to previous behavior or what explains the change?

This is a known issue. We have seen this in recent test runs. There is no content-wise connection to the changes of this PR. For full transparency, I added the figures for CAZ and Global.

So it is back to previous behavior and it just looked better in-between because of bugs / by chance?

FelicitasBeier · 2024-01-16T09:46:44Z

modules/32_forestry/dynamic_feb21/equations.gms

@@ -101,7 +101,7 @@ sum(ac_est, v32_land(j2,"aff",ac_est)) =l= sum(ac, v32_land(j2,"aff",ac)) - sum(

 q32_land_expansion(j2,type32) ..
    v32_land_expansion(j2,type32) =e= 
-    sum(ac_est, v32_land(j2,type32,ac_est)) - sum(ac_est, pc32_land(j2,type32,ac_est));
+    sum(ac_est, v32_land(j2,type32,ac_est));

 q32_land_reduction(j2,type32,ac_sub) ..
  v32_land_reduction(j2,type32,ac_sub) =e= pc32_land(j2,type32,ac_sub) - v32_land(j2,type32,ac_sub);


Just to make sure: the pc32_lanc(j2,type32,ac_est) is no longer needed in q32_land_expansion, but in q32_land_reduction it still enters?

No. But ac_est and ac_sub are dynamic sets of ac, depending on the time step length. It is not possible to define parameters, variables and equations for dynamics sets. Therefore, pc32_land is defined over ac.

tscheypidi · 2024-01-16T09:47:55Z

It looks a bit like a bi-stability where the model sometimes runs in one local optimum and the other time in the other one. Not nice, but most likely also not related to this PR

flohump · 2024-01-16T09:58:54Z

The emissions and tau behavior in CAZ is quite extreme. Where does it come from? Was it like this before and we just return to previous behavior or what explains the change?

This is a known issue. We have seen this in recent test runs. There is no content-wise connection to the changes of this PR. For full transparency, I added the figures for CAZ and Global.

So it is back to previous behavior and it just looked better in-between because of bugs / by chance?

Actually, CAZ is now (green line) closer to FAO, which is what we expect. However, as indicated by @tscheypidi this is only by chance.

pvjeetze · 2024-01-16T15:52:32Z

For me it looks like it would be sensible to also recalibrate the land conversion costs here and update the calibration tgz. The cropland trajectory seems a bit off from the historical trend.

pvjeetze

From this PR alone it does not become clear yet, why the switch from (sum(cell(i2,j2), f44_biome(j2,biome44)) > 0) to (sum(cell(i2,j2), f44_biome(j2,biome44)) > 1e-10) became necessary. Could you add a brief explanation PR?

emolinab · 2024-01-17T07:32:01Z

scripts/start/extra/recalibrate_realizations.R

Thanks for cleaning up the calibration scripts! :)

flohump · 2024-01-17T08:20:28Z

For me it looks like it would be sensible to also recalibrate the land conversion costs here and update the calibration tgz. The cropland trajectory seems a bit off from the historical trend.

I ran the calibration for land conversion costs. It does not change anything. The results for calib_run are virutally identical to PR624_after2.
Altough cropland looked better at global scale in PR624_before, oil crop demand and area in CAZ were completely out of range. Oil crops look much better in PR624_after2. However, the changes in this PR were not itended to change any of these variables.

flohump · 2024-01-17T08:23:58Z

From this PR alone it does not become clear yet, why the switch from (sum(cell(i2,j2), f44_biome(j2,biome44)) > 0) to (sum(cell(i2,j2), f44_biome(j2,biome44)) > 1e-10) became necessary. Could you add a brief explanation PR?

Changed condition from 0 to 1e-10 to avoid division by zero. We use 1e-10 and 1e-6 in multiple other places in the model to avoid division by zero.

pvjeetze

Looks fine for me!

flohump · 2024-01-17T14:44:32Z

From this PR alone it does not become clear yet, why the switch from (sum(cell(i2,j2), f44_biome(j2,biome44)) > 0) to (sum(cell(i2,j2), f44_biome(j2,biome44)) > 1e-10) became necessary. Could you add a brief explanation PR?

Changed condition from 0 to 1e-10 to avoid division by zero. We use 1e-10 and 1e-6 in multiple other places in the model to avoid division by zero.

After further discussion with @pvjeetze including an final test, we agreed that the change in the biodiversity module is not needed.

flohump added 5 commits January 15, 2024 10:48

bugfixes ac_est

f7dce1e

bugfixes ac_est

24b22c4

bugfixes ac_est

0800985

bugfix

ca7be92

bugfix

94a5319

flohump added Medium risk bugfix labels Jan 15, 2024

flohump requested review from tscheypidi and FelicitasBeier January 15, 2024 20:00

flohump marked this pull request as ready for review January 16, 2024 08:23

tscheypidi approved these changes Jan 16, 2024

View reviewed changes

FelicitasBeier requested a review from pvjeetze January 16, 2024 09:23

FelicitasBeier reviewed Jan 16, 2024

View reviewed changes

FelicitasBeier requested a review from emolinab January 16, 2024 11:49

pvjeetze reviewed Jan 16, 2024

View reviewed changes

clean up recalibration scripts

1b6d0df

emolinab reviewed Jan 17, 2024

View reviewed changes

pvjeetze approved these changes Jan 17, 2024

View reviewed changes

flohump added 2 commits January 17, 2024 12:39

FSEC calibration script

4224f7b

revert back bii condition

8910022

emolinab approved these changes Jan 18, 2024

View reviewed changes

flohump merged commit 5c06558 into magpiemodel:develop Jan 18, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bugfixes ac_est #624

bugfixes ac_est #624

flohump commented Jan 15, 2024 •

edited

Loading

FelicitasBeier Jan 16, 2024

FelicitasBeier Jan 16, 2024

flohump Jan 16, 2024

emolinab Jan 17, 2024 •

edited

Loading

flohump Jan 17, 2024 •

edited

Loading

emolinab Jan 17, 2024

flohump Jan 17, 2024

FelicitasBeier left a comment

flohump commented Jan 16, 2024 •

edited

Loading

FelicitasBeier commented Jan 16, 2024

FelicitasBeier Jan 16, 2024

flohump Jan 16, 2024

tscheypidi commented Jan 16, 2024

flohump commented Jan 16, 2024

pvjeetze commented Jan 16, 2024

pvjeetze left a comment

emolinab Jan 17, 2024

flohump commented Jan 17, 2024 •

edited

Loading

flohump commented Jan 17, 2024

pvjeetze left a comment

flohump commented Jan 17, 2024

bugfixes ac_est #624

bugfixes ac_est #624

Conversation

flohump commented Jan 15, 2024 • edited Loading

🐦 Description of this PR 🐦

🔧 Checklist for PR creator 🔧

📉 Performance changes 📈

🚨 Checklist for reviewer 🚨

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emolinab Jan 17, 2024 • edited Loading

Choose a reason for hiding this comment

flohump Jan 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FelicitasBeier left a comment

Choose a reason for hiding this comment

flohump commented Jan 16, 2024 • edited Loading

FelicitasBeier commented Jan 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tscheypidi commented Jan 16, 2024

flohump commented Jan 16, 2024

pvjeetze commented Jan 16, 2024

pvjeetze left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flohump commented Jan 17, 2024 • edited Loading

flohump commented Jan 17, 2024

pvjeetze left a comment

Choose a reason for hiding this comment

flohump commented Jan 17, 2024

flohump commented Jan 15, 2024 •

edited

Loading

emolinab Jan 17, 2024 •

edited

Loading

flohump Jan 17, 2024 •

edited

Loading

flohump commented Jan 16, 2024 •

edited

Loading

flohump commented Jan 17, 2024 •

edited

Loading