-
Notifications
You must be signed in to change notification settings - Fork 164
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
34_urban exo_nov21 Realisation PR #340
Conversation
@caviddhen codeCheck failed....please fix the issue first |
Ah sorry, Fixed now |
*' Urban land is fixed - turning off for urban land expansion implementation | ||
*' v10_lu_transitions.fx(j,land_from10,"urban") = 0; | ||
*' v10_lu_transitions.fx(j,"urban",land_to10) = 0; | ||
*' v10_lu_transitions.fx(j,"urban","urban") = pcm_land(j,"urban"); |
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.
two issues/questions:
- using
*'
will make your comment a goxygen comment and it will show up in the document. Is that intended? - As this was fixed before but is not fixed anymore: This realization will still work with static urban?
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.
Ah, I can just delete it instead?
Static urban still static. I believe this line in static realization already constrains it, the results also maintain static urban land:
https://github.com/magpiemodel/magpie/blob/rc/modules/34_urban/static/presolve.gms#L9
*' Cost terms for when vm_land(j2,"urban") is less than and greater than the input | ||
|
||
q34_urban_cost1(j2) .. | ||
v34_cost1(j2) =g= sum(ct, i34_urban_area(ct, j2)) - vm_land(j2,"urban"); |
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.
Why is that required? why not just use vm_land.fx(j2,"urban") = i34_urban_area(ct,j2)
?
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.
We thought that this way safeguards against infeasible outcomes. Where urban land should expand but can not due to NPI or other protected land constraints. In this case it can have high costs and shift it elsewhere in the region.
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.
should be added to the explanation text above
# | AGPL-3.0, you are granted additional permissions described in the | ||
# | MAgPIE License Exception, version 1.0 (see LICENSE file). | ||
# | Contact: magpie@pik-potsdam.de | ||
name,type,reason |
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.
please do not add empty not_used files. Remove it, if everything is being used.
CHANGELOG.md
Outdated
@@ -8,6 +8,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |||
|
|||
## [Unreleased] | |||
|
|||
- **34_urban** New exo_nov21 exogenous realization of urban land expansion |
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.
This needs to put under the sub-category "added"
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.
once the runtime issue is clarified and my minor comments were corrected, ready to merge!
config/default.cfg
Outdated
@@ -1424,6 +1424,7 @@ cfg$files2export$start <- c("input/info.txt", | |||
"modules/30_crop/endo_apr21/input/avl_cropland_0.5.mz", | |||
"modules/50_nr_soil_budget/input/f50_NitrogenFixationRateNatural_0.5.mz", | |||
"modules/50_nr_soil_budget/input/f50_AtmosphericDepositionRates_0.5.mz", | |||
"input/f34_UrbanLand_0.5.mz", |
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.
careful with large and small letters in filenames; I think this can lead to problems. I would suggest only small letters and to use _ instead
@@ -6,7 +6,7 @@ | |||
*** | Contact: magpie@pik-potsdam.de | |||
|
|||
scalars | |||
s10_cost_balance Artificial cost for balance variable (USD05MER per ha) / 1000000 / | |||
s10_cost_balance Artificial cost for balance variable (USD05MER per ha) / 1e+06 / |
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.
maybe 1e+5 is sufficient. Not sure whether high costs are good (punishmnet) or bad (scaling9
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.
strange name "s10_cost_balance"; shouldnt it be rather "s10_punishment_area_deviation" or something like that?
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.
This is not one of my changes. It is just a renaming of the unit, I think it comes from Florian's branch when i cloned it.
*' Sum up cost terms with high punishment | ||
|
||
q34_urban_cell(j2) .. | ||
vm_cost_urban(j2) =e= (v34_cost1(j2) + v34_cost2(j2)) * 1000000; |
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.
shouldnt the 1000000 be s10_cost_balance?
*' Cost terms for when vm_land(j2,"urban") is less than and greater than the input | ||
|
||
q34_urban_cost1(j2) .. | ||
v34_cost1(j2) =g= sum(ct, i34_urban_area(ct, j2)) - vm_land(j2,"urban"); |
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.
should be added to the explanation text above
modules/34_urban/exo_nov21/input.gms
Outdated
*** | MAgPIE License Exception, version 1.0 (see LICENSE file). | ||
*** | Contact: magpie@pik-potsdam.de | ||
|
||
table f34_UrbanLand(t_all, j, gdp_scen09) Urban 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.
also here, avoid large letters in gams code
*** | Contact: magpie@pik-potsdam.de | ||
|
||
vm_carbon_stock.fx(j,"urban",c_pools) = 0; | ||
*' Biodiveristy value (BV) |
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.
spelling
|
||
vm_carbon_stock.fx(j,"urban",c_pools) = 0; | ||
*' Biodiveristy value (BV) | ||
vm_bv.fx(j,"urban", potnatveg) = pcm_land(j,"urban") * fm_bii_coeff("urban",potnatveg) * fm_luh2_side_layers(j,potnatveg); |
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.
this is exisiting code, but all these biodiv variable names are quite cryptic... no idea what side-layers should mean, and bv=biodiv is not clear, and bii is also only clear to insiders...
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.
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 agree, there is room for improvement. Also the "Bending the curve" paper should be added as reference. But this should be done as part of a dedicated PR, not as part of this PR.
*' Cellular level is "flexibly" prescribed in that there is a very high punishment term for deviating from original input values. | ||
*' Regional sums of urban land must add be equal for both model and input. | ||
|
||
*' @limitations Urban land is exogenously prescribed and does not interact with other model dynamics. |
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.
cellular levels...
and
@limitations...
these two sentences seem to contradict each other. is it flexibel or exo prescribed?
modules/34_urban/module.gms
Outdated
@@ -11,8 +11,9 @@ | |||
*' (see also the other land modules: [30_crop], [31_past], [32_forestry], [35_natveg]). | |||
*' It describes urban settlement areas and estimates their corresponding carbon content. | |||
*' | |||
*' @authors Jan Philipp Dietrich, Florian Humpenöder | |||
*' @authors Jan Philipp Dietrich, Florian Humpenöder, David Chen |
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 want to be part here too at the end...
*' Sum up cost terms with high punishment | ||
|
||
q34_urban_cell(j2) .. | ||
vm_cost_urban(j2) =e= (v34_cost1(j2) + v34_cost2(j2)) * 1000000; |
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.
1000000 should be added as scalar like s34_cost_urban_deviation
🐦 Purpose of this PR 🐦
🔧 Checklist for PR creator 🔧
📉 Performance loss/gain from current default behavior 📈
![urban_exo](https://user-images.githubusercontent.com/37618991/140606441-7fe78106-ba7a-4f37-b465-b4db3839bbfa.png)
Added changes to
CHANGELOG.md
Compilation check (model starts without compilation errors - use
gams main.gms action=c
in model folder for testing).No hard coded numbers and cluster/country/region names.
The new code doesn't contain declared but unused parameters or variables.
Where relevant, In-code comments added including documentation comments.
Made sure that documentation created with
goxygen
is okay (usegoxygen::goxygen()
for testing).Changes to
magpie4
R library for post processing of model output (ideally backward compatible).Self-review of my own code.
NA
🚨 Checklist for RSE reviewer 🚨
CHANGELOG
is updated correctly🚨 Checklist for MAgPIE reviewer 🚨
CHANGELOG
is updated correctly