Croparea update - #13
Draft
FelicitasBeier wants to merge 39 commits into
Draft
Conversation
… argument to optionally return fallow land as element in crop set
…opareaUpdate # Conflicts: # .buildlibrary # CITATION.cff # DESCRIPTION # README.md
Merge branch 'main' into cropareaUpdate # Conflicts: # .buildlibrary # .github/workflows/pkgdown.yaml # CITATION.cff # DESCRIPTION # README.md
This reverts commit 2c147b7.
This reverts commit 87128ac.
… from calcForestArea to get consistent forest area (including no primary forest growth) for all function calls,
This comment was marked as resolved.
This comment was marked as resolved.
This was referenced Aug 6, 2026
calcFallow() defaults to cellular = FALSE and then returns country-level data, but hardcoded isocountries = FALSE. madrat therefore skipped its ISO-country validation and would have mis-handled country-to-region aggregation for any aggregate = TRUE caller. Set isocountries = !cellular, matching the sibling calcCroparea() and the other country-capable calc functions in this package. Behaviour change: the one live country-level caller, mrvalidation calcValidCroparea() (cellular = FALSE, aggregate = FALSE), now passes through madrat's ISO-set check. No caller uses aggregate = TRUE. The cellular = TRUE caller (calcValidCropareaDiversity) is unaffected. Addresses review finding M10.
…nally Same defect class as M10: both functions aggregate to country level in their !cellular branch (dimSums over x/y followed by toolCountryFill), but hardcoded isocountries = FALSE. madrat therefore skipped its ISO-country validation on the country-level path and would have mis-handled country-to-region aggregation for any aggregate = TRUE caller. Set isocountries = !cellular in both, matching calcCroparea and calcFallow. calcCropareaLandInG defaults to cellular = FALSE, so this is a live change: the country-level callers mrvalidation calcValidCroparea() (line 35) and calcValidYield() (line 129) now pass through madrat's ISO-set check. All other callers in mrlandcore, mrland and mrwater pass cellular = TRUE explicitly and are unaffected. calcFallowLandInG defaults to cellular = TRUE, so its default behaviour is unchanged; both known callers (calcCropareaLandInG line 52, calcCroparea line 67) pass cellular = TRUE. This one is a latent fix. Found by grepping for the M10 pattern after fixing M10; not in the original review.
The branch at calcCroparea.R:37 (irrigation && cellular && fallow) appended " including fallow land." to the description, and the unconditional block at lines 143-148 appended it again, since that branch requires fallow = TRUE. The description read "... including fallow land. including fallow land." Removed the redundant append; the later block covers every LandInG path. Cosmetic only - no effect on values, units, dimensions or weights. Addresses review finding m5.
…culation of calcCropareaFAOLUH Merge branch 'main' into cropareaUpdate # Conflicts: # .buildlibrary # CITATION.cff # DESCRIPTION # README.md
…LUH2MAgPIE should be based on croparea from FAOLUH
The cap at 3 harvests per year is unchanged - the returned data is identical. What is new is a comment giving the rationale and a conditional message. Rationale, per the author: inconsistencies between the physical and the harvested area data set can produce unrealistically high multicropping factors in edge cases, and three harvests per year is the defensible ceiling. 10 Mha is where an edge case stops being an edge case. The physical area affected is measured from the uncapped factor, weighted by physical croparea, summed globally per year; if the maximum across years exceeds 10 Mha a vcat(1, ...) names that area and that year. Only reachable under physical = FALSE, where the factor is computed at all. na.rm = TRUE in the dimSums is load-bearing, not noise: dimSums defaults to na.rm = FALSE, so a single NA would make the total NA and the following if (max(...) > 10) would error inside calcCroparea. Addresses review finding m16.
Contributor
Author
|
All issues raised by AI reviewer have been addressed. I will mark the comment as "resolved" to not confuse human reviewers. |
…sed in calcCroparea and to ensure that primary forest doesn't grow
…ion from 1960 onwards
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Updated default croparea to LandInG data.
calcCroparea can now select between LandInG croparea and old croparea from FAO-LUH.
Default is LandInG.
The crop pool at grid cell level is in line with calcLanduseInitialisation. We only use croparea patterns (and harvested/physical shares) from LandInG.