-
Notifications
You must be signed in to change notification settings - Fork 20
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
Master mppalves #33
Master mppalves #33
Conversation
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.
from rse perspective the changes look good to me, but I cannot judge the changes from a content perspective. It would be important to hear from @k4rst3ns whether the changes are in line with the other functions. Please wait for her approval before merging.
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 looks in principle fine to me (see my comments above and please adjust the functions).
One thing I noticed:
You call PastureSuit in calcPastrTauHist.R
but it is never used in there.
Line 23 in 5b6edca
pastr_weight <- calcOutput("PastureSuit", # nolint |
Please remove the call there, as this calculation is rather costly and maybe not needed at that spot.
I couldn't find another spot except for the fullCellularmagpie, where calcPastureSuit is called, nevertheless it might make sense to add the same handling of the climatetype argument as in calcYieldsLPJmL:
https://github.com/pik-piam/mrland/blob/beaafd170ff2989a7e0917a9a23ce40fccf84de7/R/calcYieldsLPJmL.R#L27
... that helps to set the correct stage depending on the climatetype setting (and if historical climatetype is used stage is set "only" to smoothed). the stage argument then has to be set in the mpet and precipitation call.
|
||
# pasture drivers | ||
calcPastureSuit <- function(datasource = "ISIMIP3bv2", climatetype = "MRI-ESM2-0:ssp126", | ||
lpjml = "LPJmL4_for_MAgPIE_44ac93de", smoothPrecipitation = 10, smoothOut = 10) { |
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.
You don't need 'smoothPrecipitation' and 'smoothOut' anymore.
Remove it as parameters here and in the header.
Also the datasource argument is a bit unclear to me. It isn't used anymore, right?
This is now mainly handled by the toolClimateInputVersion and is set automatically (depending on the LPJmL version in the future and for now to a given default). Also consider removing it (but please check maybe I missed where it is used).
#' on population and aridity criteria. | ||
#' @param datasource Document | ||
#' @param climatetype Document | ||
#' @param lpjml Document |
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.
What does the "Document" here stands for? ^^'
|
||
cellPrep <- calcOutput("LPJmLClimateInput", climatetype = climatetype, | ||
variable = "precipitation:monthlySum", | ||
stage = "smoothed", |
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.
stage has to be 'harmonized2020' (as this is future data, right?)
|
||
cellPet <- calcOutput(type = "LPJmL_new", climatetype = climatetype, | ||
subtype = "mpet", | ||
stage = "smoothed", |
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.
stage has to be 'harmonized2020' (as this is future data, right?)
@@ -206,12 +206,13 @@ fullCELLULARMAGPIE <- function(rev = 0.1, dev = "", | |||
subtype = "/co2/Nreturn0p5", # nolint | |||
lsu_levels = c(seq(0, 2.2, 0.2), 2.5), past_mngmt = "mdef", | |||
file = paste0("f31_grassl_yld.mz"), years = magYears, aggregate = FALSE) | |||
calcOutput("PastureSuit", subtype = paste("ISIMIP3bv2", "MRI-ESM2-0", "1850_2100", sep = ":"), | |||
calcOutput("PastureSuit", datasource = "ISIMIP3bv2", climatetype = "MRI-ESM2-0:ssp126", |
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.
climatetype and lpjml argument has to be set to the flexible arguments as they change with the preprocessing settings.
See e.g.:
mrmagpie/R/fullCELLULARMAGPIE.R
Line 322 in 5b6edca
calcOutput("Carbon", aggregate = FALSE, lpjml = lpjml, climatetype = climatetype, |
My guess how the call should look like:
calcOutput("PastureSuit", climatetype = climatetype, lpjml = lpjml[["natveg"]],
file = paste0("f31_pastr_suitability_", ctype, ".mz"), years = lpjYears, aggregate = "cluster")
Check which years make most sense (magYears or lpjYears) and the lpjml version ('natveg' or 'grass').
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.
sorry also added another comment ^^'
yearsCellPet <- intersect(getYears(cellPet), findset("time")) | ||
yearsCellPrep <- intersect(findset("time"), getYears(cellPrep)) | ||
years <- intersect(yearsCellPet, yearsCellPrep) | ||
cellPrep <- dimSums(cellPrep[, years, ], dim = 3) |
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.
It seems you only need yearly precipitation and pet, is this correct?
We might than already call the function for yearly sums as this would speed up harmonization, but might smooth out more extreme yearly fluctuations. Maybe we ask Jens here once again, what makes sense.
|
||
# Cell area calculation | ||
landcoords <- as.data.frame(toolGetMapping("magpie_coord.rda", type = "cell", where = "mappingfolder")) | ||
landcoords <- cbind(landcoords, rep(1, nrow(landcoords))) | ||
landcoords <- raster::rasterFromXYZ(landcoords) | ||
crs(landcoords) <- "+proj=longlat" # outputs cell are in km2 | ||
raster::crs(landcoords) <- "+proj=longlat" # outputs cell are in km2 |
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.
Do you really need cellsize or landarea size of a given cell?
If you need this to calculate out of total numbers density - maybe landarea size per cell makes more sense?
there is a function already calculating this:
calcOutput("CellArea", aggregate = FALSE)
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.
Last comment:
PastureSuit is now flexibel with climate and lpjml version.
For GrasslandYields this is still to be done.
Added new PastureSuit function (still need to implement 67k cells)