bugfixes for more robust feasibility #744

flohump · 2024-11-05T15:05:13Z

🐦 Description of this PR 🐦

This PR aims to fix infeasibilies that I have repeatedly encountered for single runs when testing the current develop with the full set of FSEC runs and runs with H16 resolution.
The fix in 10_land for the land transtion matrix is to use only variables (vm_land) and variables levels (vm_land.l), and not a mix of variables and parameters. The issue is that GAMS seems to have different accuracies for variables and parameters. However, vm_land.l is updated between time steps based on parameters. Therefore, the balance variables are still needed.

This PR has been successfully tested with test_runs.R, the full set of FSEC runs and some H16EU runs with rev4.114. Tests with rev4.115 are underway.

10_land bugfix land transition matrix for improved feasibility (variables and parameters have different accuracy)
44_biodiversity avoid division by zero and improved consistency between realisations for fixing variables
70_livestock bugfix scaling.gms file in wrong folder
modules update of scaling factors in several modules

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

modules/44_biodiversity/bii_target_apr24/presolve.gms

pascal-sauer

Thanks for fixing these infeasibilities!

pascal-sauer · 2024-11-06T13:11:17Z

modules/10_land/landmatrix_dec18/scaling.gms

-v10_balance_positive.scale(j) = 10e-10;
-v10_balance_negative.scale(j) = 10e-10;
+v10_balance_positive.scale(j,land_from) = 10e-7;
+v10_balance_negative.scale(j,land_from) = 10e-7;


Can you explain what is happening here? Why does it help to switch to 10^-7?

I ran gdx2::calcScaling(gdx) on a couple of different test runs. For most of them the suggestion is to use 10^-7 for scaling.

Okay this may be more of an understanding question, so the scaling tells gams the order of magnitude of step lengths when iterating towards a local optimum?

https://www.gams.com/latest/docs/S_CONOPT4.html#CONOPT4_SCALING
internal_model = user_model / scaling_factor
GAMS is also doing automatic scaling but the default for Tol_Scale_Min is 1. Since we have manual scaling factors < 1 I think we should use Tol_Scale_Min = 1.e-10; in the optFile.
https://www.gams.com/latest/docs/S_CONOPT4.html#CONOPTTol_Scale_Min

pascal-sauer · 2024-11-06T13:13:01Z

modules/44_biodiversity/bii_target/presolve.gms

@@ -23,7 +23,7 @@ else
  );
  p44_bii_lower_bound(t2,i,biome44)$(p44_bii_lower_bound(t2,i,biome44) >= 1) = 1;
  p44_bii_lower_bound(t2,i,biome44)$(m_year(t2) < s44_start_year) = 0;
-  p44_bii_lower_bound(t2,i,biome44)$(sum(cell(i,j), f44_biome(j,biome44)) = 0) = 0;
+  p44_bii_lower_bound(t2,i,biome44)$(sum(cell(i,j), f44_biome(j,biome44)) <= 1e-10) = 0;


Why is this change (and other similar changes where 0 is replaced with 1e-10) necessary?

In this specific case for bii_target it was inconsistent with presolve and equations, where it is already 1e-10.
The intention is to fix v44_bii to zero in case the biome share f44_biome is very small. And this case also the lower bound should be zero.
I implemented the same logic for the bii_target_apr24 realisation.

I'm not convinced. Replacing small values with 0 / treating zeros the same as tiny numbers seems like losing control. We don't quite know why this works, but if we replace it with zero it works, and it's almost zero anyway... I have a bad feeling about this practice, we are piling up ultra hard to debug problems in our code this way is my feeling. This is of course more a general comment and not specific to this instance/PR.

I don't understand this point. Why should it be an issue to treat tiny numbers like zero?
My understanding is that such tiny numbers might cause issues for the solver.
We also do this in other parts of the model.
For debugging we have off / simple module realizations in many cases.
Do you have another suggestion for addressing this problem?

Why should it be an issue to treat tiny numbers like zero?

It's just mathematically wrong, and we don't understand the implications. This lack of control what's going on is what's bugging me, but that is of course inherent to using gams/conopt4, so not something we can change easily :/

We also do this in other parts of the model.

That is not a great argument and just makes me feel more anxious.

Do you have another suggestion for addressing this problem?

Unfortunately no, I cannot follow what is happening really, so I'll approve. Wanted to emphasize my concerns though.

Would be interested in the other reviewers perspective on this @tscheypidi @FelicitasBeier @k4rst3ns :) Am I just worrying to much? 🙈

I totally get your concern. It has been a common practice in the past, though. So maybe something to discuss in the next MAgPIE meeting?

Same concern here.

On Florians:

My understanding is that such tiny numbers might cause issues for the solver

... I would not know why theoretically and generally this should be an issue.
We observed that over the years, but do not have a great understanding here.

But I am also see @flohump point: this is not specific to this PR. And I like @FelicitasBeier suggestion bringing this up to the MAgPIE meeting and approving this for now.

FelicitasBeier · 2024-11-07T13:59:02Z

modules/10_land/landmatrix_dec18/equations.gms


 q10_transition_to(j2,land_to) ..
  sum(land_from, vm_lu_transitions(j2,land_from,land_to)) =e=
  vm_land(j2,land_to);

 q10_transition_from(j2,land_from) ..
  sum(land_to, vm_lu_transitions(j2,land_from,land_to)) =e=
-  pcm_land(j2,land_from);
+  vm_land.l(j2,land_from) + v10_balance_positive(j2,land_from) - v10_balance_negative(j2,land_from);


I don't know this formulation from equations. Thought that's a presolve/postsolve thing. Are you sure that works?

And in the above two equations, you really want the variable and not the level, right? Just to make sure because the first one (q10_transition_matrix) has changed from pcm_land to vm_land, not vm_land.l

I don't know this formulation from equations. Thought that's a presolve/postsolve thing. Are you sure that works?

Yes, it works ;-)

And in the above two equations, you really want the variable and not the level, right? Just to make sure because the first one (q10_transition_matrix) has changed from pcm_land to vm_land, not vm_land.l

I was thinking about the reasons for the infeasible solutions and came to the conclusiong that it might be due to inconsistencies in these 3 equations from the use of vm_land and pcm_land.
Therefore, I switched to vm_land in the first two equations and vm_land.l in third equation.

I don't fully understand why, but I am happy the infeasibilities are gone, so I'm gonna approve.

k4rst3ns

Not too much understanding of the changes I believe.
Happy to get feedback on the pcm_land to vm_land.l change before approval (sorry 😅 )

k4rst3ns · 2024-11-06T11:18:58Z

modules/10_land/landmatrix_dec18/equations.gms


 q10_transition_to(j2,land_to) ..
  sum(land_from, vm_lu_transitions(j2,land_from,land_to)) =e=
  vm_land(j2,land_to);

 q10_transition_from(j2,land_from) ..
  sum(land_to, vm_lu_transitions(j2,land_from,land_to)) =e=
-  pcm_land(j2,land_from);
+  vm_land.l(j2,land_from) + v10_balance_positive(j2,land_from) - v10_balance_negative(j2,land_from);


I understand that vm_land.l should suppose to be the same as pcm_land.
But as you also pointed out pcm_land is actually changed in some resolve functions, so this is not at all identical or not?

I am not 100% sure why we need this balancing, if we could just define a variable with a level that has the correct value of pcm_land?

In this instance it does not make a difference if we use pcm_land or vm_land.l. They should be identical.
The point is that vm_land.l is changed in the presolve based on parameters, which seem to have a different accuracy than the variable.
I tried without the balancing variables. But without them some of the test runs are infeasible.

k4rst3ns · 2024-11-07T13:56:17Z

modules/44_biodiversity/bii_target/presolve.gms

@@ -23,7 +23,7 @@ else
  );
  p44_bii_lower_bound(t2,i,biome44)$(p44_bii_lower_bound(t2,i,biome44) >= 1) = 1;
  p44_bii_lower_bound(t2,i,biome44)$(m_year(t2) < s44_start_year) = 0;
-  p44_bii_lower_bound(t2,i,biome44)$(sum(cell(i,j), f44_biome(j,biome44)) = 0) = 0;
+  p44_bii_lower_bound(t2,i,biome44)$(sum(cell(i,j), f44_biome(j,biome44)) <= 1e-10) = 0;


Same concern here.

On Florians:

My understanding is that such tiny numbers might cause issues for the solver

... I would not know why theoretically and generally this should be an issue.
We observed that over the years, but do not have a great understanding here.

But I am also see @flohump point: this is not specific to this PR. And I like @FelicitasBeier suggestion bringing this up to the MAgPIE meeting and approving this for now.

flohump added 28 commits November 4, 2024 06:15

Tol_Feas_Min = 3.e-13

b99affc

update

b86f689

test

141a0bc

update

509595d

update

8897503

bugfix

f8348d7

bugfix

47a8258

bugfix

65ccea5

test

9a1e2b9

test

43f1357

update

1e439cd

update

a07bd31

test

1fac4e5

bugfix

6a8a23f

update

492c783

fix

99e0890

update

d905f25

update

a3743a6

update

a378239

test

ba9483d

update

af74fd2

test

d386fb1

bugfix

47f3227

update

fe8cd67

test

84436ed

bugfix

815f223

Merge branch 'develop' of github.com:magpiemodel/magpie into f_fixOpti

7cd247a

changelog

6592c04

flohump added enhancement New feature or request Low risk Low risk labels Nov 6, 2024

flohump added bugfix and removed Low risk Low risk labels Nov 6, 2024

flohump requested review from pascal-sauer and tscheypidi November 6, 2024 09:28

flohump marked this pull request as ready for review November 6, 2024 09:29

flohump requested a review from FelicitasBeier November 6, 2024 09:33

FelicitasBeier requested a review from k4rst3ns November 6, 2024 09:37

FelicitasBeier reviewed Nov 6, 2024

View reviewed changes

modules/44_biodiversity/bii_target_apr24/presolve.gms Outdated Show resolved Hide resolved

bugfix typo

aa034ef

flohump requested a review from FelicitasBeier November 6, 2024 13:01

pascal-sauer reviewed Nov 6, 2024

View reviewed changes

update start scripts

1a84ac8

flohump requested a review from pascal-sauer November 7, 2024 10:03

pascal-sauer approved these changes Nov 7, 2024

View reviewed changes

FelicitasBeier reviewed Nov 7, 2024

View reviewed changes

k4rst3ns reviewed Nov 7, 2024

View reviewed changes

FelicitasBeier approved these changes Nov 7, 2024

View reviewed changes

flohump added 2 commits November 8, 2024 12:57

remove scaling for 29_cropland

3323a4e

start script update counter

0ebe136

flohump merged commit 321bea6 into magpiemodel:develop Nov 11, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bugfixes for more robust feasibility #744

bugfixes for more robust feasibility #744

flohump commented Nov 5, 2024 •

edited

Loading

pascal-sauer left a comment

pascal-sauer Nov 6, 2024

flohump Nov 6, 2024

pascal-sauer Nov 7, 2024

flohump Nov 7, 2024

pascal-sauer Nov 6, 2024

flohump Nov 6, 2024

pascal-sauer Nov 7, 2024

flohump Nov 7, 2024

pascal-sauer Nov 7, 2024

pascal-sauer Nov 7, 2024

FelicitasBeier Nov 7, 2024

k4rst3ns Nov 7, 2024

FelicitasBeier Nov 7, 2024

FelicitasBeier Nov 7, 2024

flohump Nov 7, 2024

flohump Nov 7, 2024

FelicitasBeier Nov 7, 2024

k4rst3ns left a comment

k4rst3ns Nov 6, 2024

flohump Nov 7, 2024

k4rst3ns Nov 7, 2024

bugfixes for more robust feasibility #744

bugfixes for more robust feasibility #744

Conversation

flohump commented Nov 5, 2024 • edited Loading

🐦 Description of this PR 🐦

🔧 Checklist for PR creator 🔧

📉 Performance changes 📈

🚨 Checklist for reviewer 🚨

pascal-sauer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k4rst3ns left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flohump commented Nov 5, 2024 •

edited

Loading