-
Notifications
You must be signed in to change notification settings - Fork 173
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
Peatland rewetting automatically considered during SEALS downscaling #759
Conversation
) | ||
sealsCoeff <- suppressWarnings(sealsCoeff[min(which(file.exists(sealsCoeff)))]) |
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 seems rather hacky, is there no way of knowing where the file will actually be?
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 approach is pretty common in post-processing functions mostly in the magpie4
library, from which I have transferred part of the code. This makes the code more stable and less susceptible depending on which relative directory the code may be run.
Would you have an alternative suggestion?
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.
My suggestion would be to not run the code from different relative directories / setup up the whole system in a way such that that is not necessary. Not something that can be solved in this script though.
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 a bit nicer this way:
sealsCoeff <- suppressWarnings(sealsCoeff[min(which(file.exists(sealsCoeff)))]) | |
sealsCoeff <- Find(file.exists, sealsCoeff) |
This returns NULL if no file exists, so would need to change line 160 to !is.null
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'd suggest the same for seals config below as well
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.
Thanks for including peatlands in SEALS!
I have one request for adjusting the criterion when peatlands are activatd in SEALS.
"WDPA", consv, sealsCoeff[consvRow, "data_location"] | ||
) | ||
|
||
peatlandPrice <- readGDX(file.path(dir, "fulldata.gdx"), "im_pollutant_prices") |
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 suggest to use a different criterion for activating peatlands in SEALS.
We now also have to option for external peatland rewetting scenarios.
Therefore, I suggest to use rewetted peatland area as criterion, as a share of total peatland area.
dimSums(PeatlandArea(gdx)[,,"rewet"],dim=1)/dimSums(PeatlandArea(gdx),dim=c(1,3))>0.01
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.
Thanks for spotting this and the suggestion! I've changed the code based on your suggestion.
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.
Looks good. Thanks!
) | ||
sealsCoeff <- suppressWarnings(sealsCoeff[min(which(file.exists(sealsCoeff)))]) |
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 a bit nicer this way:
sealsCoeff <- suppressWarnings(sealsCoeff[min(which(file.exists(sealsCoeff)))]) | |
sealsCoeff <- Find(file.exists, sealsCoeff) |
This returns NULL if no file exists, so would need to change line 160 to !is.null
sealsCoeff <- c( | ||
"input/seals_global_coefficients.csv", | ||
"../input/seals_global_coefficients.csv", | ||
"../../input/seals_global_coefficients.csv" | ||
) |
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.
sealsCoeff <- c( | |
"input/seals_global_coefficients.csv", | |
"../input/seals_global_coefficients.csv", | |
"../../input/seals_global_coefficients.csv" | |
) | |
sealsCoeff <- paste0(c("./", "../", "../../"), "input/seals_global_coefficients.csv") |
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.
Thanks Pascal for these simplifications! I've reworked the parts of the code. Please double check.
) | ||
sealsCoeff <- suppressWarnings(sealsCoeff[min(which(file.exists(sealsCoeff)))]) |
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'd suggest the same for seals config below as well
🐦 Description of this PR 🐦
extra/runSEALSallocation.R
so that peatland rewetting is automatically considered during a SEALS allocation run.🔧 Checklist for PR creator 🔧
Label pull request from the label list.
Self-review own code
magpie4
R library has been updated accordingly and backwards compatible where necessary.scenario_config.csv
has been updated accordingly (important ifdefault.cfg
has been updated)Document changes
CHANGELOG.md
goxygen::goxygen()
and verify the modified code is properly documentedPerform test runs
Rscript start.R --> "compilation check"
Rscript start.R --> "test runs"
Rscript start.R --> "test runs"
📉 Performance changes 📈
🚨 Checklist for reviewer 🚨
CHANGELOG
is updated correctly