Skip to content

Croparea update - #18

Draft
FelicitasBeier wants to merge 2 commits into
pik-piam:mainfrom
FelicitasBeier:cropareaUpdate
Draft

Croparea update#18
FelicitasBeier wants to merge 2 commits into
pik-piam:mainfrom
FelicitasBeier:cropareaUpdate

Conversation

@FelicitasBeier

Copy link
Copy Markdown

Updated croparea to LandInG

bodirsky and others added 2 commits April 15, 2026 06:46
Merge branch 'main' into cropareaUpdate

# Conflicts:
#	.buildlibrary
#	CITATION.cff
#	DESCRIPTION
@bodirsky

bodirsky commented Aug 6, 2026

Copy link
Copy Markdown
Member

Review: mrfactors#18 (Croparea update)

Note

AI-generated review. Produced by Claude (Opus 5) running in Claude Code, at the request of
@bodirsky and posted on their behalf. They have not reviewed it in detail yet, so weigh it
accordingly. It is a static review: the diff and a snapshot of the
head branch (67c135d) were read, but no code was executed and no madrat chain was run, so any claim
that would need a run is marked as needing a check rather than asserted. Line numbers refer to that
snapshot. Please treat the findings as leads to verify, not as verdicts - and disregard anything
that misreads the intent.

Part of a coordinated review of the seven cropareaUpdate PRs - pik-piam/mrlandcore#13, pik-piam/mrcommons#206, pik-piam/mrland#74, pik-piam/mrmagpie#67, pik-piam/mrvalidation#41, pik-piam/mrwater#15, #18. Findings labelled M1-M10 (major) and m1-m17 (minor) are shared across all seven, so a reference like "see M7" points at the same issue raised on another PR.

Smallest PR in the set and the substantive change is correct.

Checked and fine

convertVittis repoints its aggregation weight from calcOutput("Croparea", sectoral = "ProductionItem", ...)
to calcOutput("CropareaFAOLUH", sectoral = "ProductionItem", ...). This is right for two reasons:
the new calcCroparea no longer supports sectoral = "ProductionItem" (so the old call would have
errored), and the Vittis crop-category mapping is FAO-based, so a FAO-consistent weight is the
appropriate one. Splitting the pipe into an intermediate temp object is cosmetic but harmless.

Major

M8 - no DESCRIPTION change. No version bump, no buildLibrary() run, and no minimum version
requirement on the mrlandcore release that first provides CropareaFAOLUH. Installed against an older
mrlandcore this PR fails with "unknown type CropareaFAOLUH". Since the whole point of the change is to
pin the weight to a function that only exists in the new mrlandcore, the version constraint matters
more here than anywhere else in the set.

Minor

  • m11 The diff is 4 files, of which 3 are tooling: .positai entries in .gitignore and
    .Rbuildignore, and a ProjectId: line in mrfactors.Rproj. Harmless, but they are unrelated to
    the croparea update and appear identically across several PRs in this set - worth splitting into one
    separate housekeeping PR rather than repeating them.

Not updated, silently switched to LandInG

calcLandRent.R:19 and calcProductionCosts.R:15 both still call calcOutput("Croparea", ...) and
now receive LandInG data. Neither file is touched by this PR. They still compile, and the switch is
probably intended, but since convertVittis was deliberately pinned to CropareaFAOLUH it is worth
saying explicitly why these two go the other way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants