-
Notifications
You must be signed in to change notification settings - Fork 169
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’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
bugfixes for more robust feasibility #744
Changes from 28 commits
b99affc
b86f689
141a0bc
509595d
8897503
f8348d7
47a8258
65ccea5
9a1e2b9
43f1357
1e439cd
a07bd31
1fac4e5
6a8a23f
492c783
99e0890
d905f25
a3743a6
a378239
ba9483d
af74fd2
d386fb1
47f3227
fe8cd67
84436ed
815f223
7cd247a
6592c04
aa034ef
1a84ac8
3323a4e
0ebe136
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,17 +17,16 @@ | |
*' `v10_balance_negative`should deviate from zero only in exceptional cases. | ||
|
||
q10_transition_matrix(j2) .. | ||
sum((land_from,land_to), vm_lu_transitions(j2,land_from,land_to)) | ||
+ v10_balance_positive(j2) - v10_balance_negative(j2) =e= | ||
sum(land, pcm_land(j2,land)); | ||
sum((land_from,land_to), vm_lu_transitions(j2,land_from,land_to)) =e= | ||
sum(land, vm_land(j2,land)); | ||
|
||
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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't know this formulation from equations. Thought that's a presolve/postsolve thing. Are you sure that works? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes, it works ;-) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't fully understand why, but I am happy the infeasibilities are gone, so I'm gonna approve. |
||
|
||
*' The following two equations calculate land expansion and land contraction based | ||
*' on the above land transition matrix. | ||
|
@@ -47,7 +46,7 @@ | |
q10_cost(j2) .. | ||
vm_cost_land_transition(j2) =e= | ||
sum(land, vm_landexpansion(j2,land) + vm_landreduction(j2,land)) * 1 | ||
+ (v10_balance_positive(j2) + v10_balance_negative(j2)) * s10_cost_balance; | ||
+ sum(land_from, v10_balance_positive(j2,land_from) + v10_balance_negative(j2,land_from)) * s10_cost_balance; | ||
|
||
*' The gross changes in land are calculated based on land expansion, land | ||
*' contraction and land changes from within the modules [35_natveg] | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,5 +6,5 @@ | |
*** | Contact: [email protected] | ||
|
||
vm_landdiff.scale = 10e3; | ||
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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you explain what is happening here? Why does it help to switch to 10^-7? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I ran There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. https://www.gams.com/latest/docs/S_CONOPT4.html#CONOPT4_SCALING |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,6 @@ | |
*** | MAgPIE License Exception, version 1.0 (see LICENSE file). | ||
*** | Contact: [email protected] | ||
|
||
vm_secondary_overproduction.scale(i,kall,kpr) = 10e-3; | ||
vm_secondary_overproduction.scale(i,kall,kpr) = 10e-4; | ||
vm_cost_processing.scale(i) = 10e5; | ||
vm_processing_substitution_cost.scale(i) = 10e4; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,5 @@ | |
*** | MAgPIE License Exception, version 1.0 (see LICENSE file). | ||
*** | Contact: [email protected] | ||
|
||
vm_costs_additional_mon.scale(i) = 10e4; | ||
*Don't scale this variable. Model is very sensitive to scaling this variable and might become infeasible or very slow. | ||
*v71_feed_balanceflow.scale(j,kforage) = 10e3; | ||
v29_treecover.scale(j,ac) = 10e-3; | ||
v29_treecover_missing.scale(j) = 10e-3; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,4 +5,6 @@ | |
*** | MAgPIE License Exception, version 1.0 (see LICENSE file). | ||
*** | Contact: [email protected] | ||
|
||
vm_cost_prod_crop.scale(i,factors) = 10e4; | ||
vm_cost_prod_crop.scale(i,factors) = 10e5; | ||
v38_investment_immobile.scale(j,kcr) = 10e3; | ||
v38_capital_need.scale(j,kcr,mobil38) = 10e3; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this change (and other similar changes where 0 is replaced with 1e-10) necessary? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In this specific case for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand this point. Why should it be an issue to treat tiny numbers like zero? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 :/
That is not a great argument and just makes me feel more anxious.
Unfortunately no, I cannot follow what is happening really, so I'll approve. Wanted to emphasize my concerns though. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would be interested in the other reviewers perspective on this @tscheypidi @FelicitasBeier @k4rst3ns :) Am I just worrying to much? 🙈 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same concern here. On Florians:
... I would not know why theoretically and generally this should be an issue. 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. |
||
* The lower bound of `v44_bii` is set to `p44_bii_lower_bound` to avoid a reduction of BII in combination with `v44_bii_missing`. | ||
v44_bii.lo(i,biome44) = p44_bii_lower_bound(t,i,biome44); | ||
display p44_bii_lower_bound; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,6 @@ | |
*** | MAgPIE License Exception, version 1.0 (see LICENSE file). | ||
*** | Contact: [email protected] | ||
|
||
vm_costs_additional_mon.scale(i) = 10e4; | ||
*Don't scale this variable. Model is very sensitive to scaling this variable and might become infeasible or very slow. | ||
*v71_feed_balanceflow.scale(j,kli_rum,kforage) = 10e3; | ||
|
||
vm_cost_prod_livst.scale(i,factors) = 10e5; | ||
vm_cost_prod_fish.scale(i) = 10e5; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this instance it does not make a difference if we use
pcm_land
orvm_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.