Skip to content

Forest loss drivers: fix the Curtis table, add GFW/Sims et al. (2025) as default source - #78

Open
mscrawford wants to merge 28 commits into
pik-piam:masterfrom
mscrawford:fix/forestlossdrivers-source
Open

mscrawford wants to merge 28 commits into
pik-piam:masterfrom
mscrawford:fix/forestlossdrivers-source

Conversation

@mscrawford

@mscrawford mscrawford commented Sep 14, 2026 •

Copy link
Copy Markdown
Contributor

f35_forest_lost_share sets the forest disturbance in MAgPIE module 35; at the default s35_forest_damage = 2 only its shifting column is read. Curtis et al. (2018) define shifting agriculture as clearing followed by regrowth, which is what module 35 models. The problems were in the transcription and the country mapping.

Curtis et al. (2018) source (ForestLossDrivers)

  • forest_loss.csv gave Africa 3 Mha of tree cover loss; Table 1 gives 39. The rows summed to 279 Mha against the printed global 314 (315 with 39). Africa is 92% shifting agriculture, so the SSA rate has been 13x too low in every input revision since December 2020.
  • mapping.csv put the 19 Middle East and North Africa countries in Latin America. The article lists no countries per region, so they are reassigned by geography: North Africa to Africa, the Middle East to Russia/China/South Asia, Reunion to Africa. Europe's 1% moves from shifting agriculture to wildfire, as printed.
  • The table stays a madrat source: the corrected table is added to the shared source folder as forest_loss_fixed.csv, which readForestLossDrivers() reads. forest_loss.csv and mapping.csv there stay unchanged for older mrland versions. The country mapping ships in inst/extdata/regional/regionmappingCurtis2018.csv. The rows are normalised to sum to 100: the printed rows sum to 99-102 (rounding, cells printed "<1%"), and each driver is total_loss * percent/100 / 15, so a row over 100 would attribute more loss than was observed.
  • calcForestLossByDriver() distributes the regions to countries by FRA 2020 naturally regenerating forest area in 2010, the area and year calcForestLossShare() divides by, so every country in a source region carries one rate.

New default source GFWLossByDriver

  • Tree cover loss by country, year and dominant driver from the GFW data-api: gadm__tcl__iso_change v20260424, i.e. UMD loss crossed with the Sims et al. (2025) 1 km driver map (by release date its annual update v1.13, 2001-2025), 30% canopy threshold, no API key.
  • calcForestFireLoss() is renamed calcForestLossByDriver(); it and calcForestLossShare() gain src ("GFW" default, "Curtis") and period (GFW only, default 2015:2024).
  • The share is loss divided by FRA 2020 naturally regenerating forest area, the natural forest MAgPIE applies it to: 2020 for GFW, 2010 for Curtis.
  • Only shifting_cultivation goes to MAgPIE, from either source; the unused FRA fire column overall is dropped (magpie#934 drops it from driver_source). Permanent agriculture, hard commodities and settlements remove the forest, logging is MAgPIE's own timber harvest, and wildfire and other natural disturbances are not land use but would be reported as land-use-change CO2. All eight classes go to validation instead (calcValidTreeCoverLoss: observed tree cover loss by driver (GFW, Sims et al. 2025) mrvalidation#45).
  • calcForestDisturbances() removed: module 35 loads f35_forest_disturbance_share but reads it nowhere.
  • Shares above 1 are still clamped to 1.

Effect on the shifting column

shifting-cultivation
share per year develop src = "Curtis" src = "GFW"
SSA 0.00028 0.00359 0.00354
IND 0 0 0.00159
OAS 0.00082 0.00082 0.00257
LAM 0.00166 0.00170 0.00027
MEA 0.00103 0.00247 0.0000001
JPN 0.00044 0.00044 0.0000001
CAZ, CHA, EUR, NEU, REF, USA < 0.0002 < 0.0002 < 0.00004

Curtis and GFW agree in SSA because that is the transcription fix. They part company where Curtis's seven regions straddle the twelve model regions: India and China sit in one source region whose shifting share rounds to zero, so no table edit can give them a rate distinct from Russia's.

Latin America carries the largest change and the weakest classification: Sims et al. report ~50% producer's accuracy for shifting cultivation there against ~80% globally.

mscrawford and others added 14 commits September 12, 2026 22:04
The Curtis et al. (2018) Table 1 transcription and its country mapping only existed
as loose files in the shared madrat source directory. They are now package data under
inst/extdata/ForestLossDrivers, staged by the download function, so the source is
reproducible and its numbers reviewable in a diff. Committed byte-for-byte as in use
(CRLF normalised, trailing newline added). SOURCE.md records the comparison against
the printed table.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FQU9MetaTSbqRJVrCypV9n
Table 1 of Curtis et al. (2018) gives Africa 39 Mha over 2001-2015; the transcription
has carried 3 since December 2020. Africa is 92% shifting agriculture, the column
MAgPIE reads by default, so the SSA rate in f35_forest_lost_share has been 13x too
low in every input revision since. Two checks confirm 39: the regional rows sum to
315 against the printed global 314 (279 with 3), and the unused share column implies
12.4% (1.1% with 3). Southeast Asia's share 12 -> 13 in the same table (unused).
checkCurtisTable1() enforces these identities on every read; verified against the
corrupt table as a positive test.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FQU9MetaTSbqRJVrCypV9n
… to wildfire

Optional relative to the Africa fix; drop this commit for a transcription-only PR.
mapping.csv was derived from regionmappingH12.csv and the edit over-ran: all 19 MEA
countries sat in "Latin America". North Africa moves to Africa, the Middle East to
"Russia China South Asia", Reunion to Africa, and SOURCE.md says so. Europe's 1% moves
from shifting agriculture to wildfire, as printed. Effect on f35_forest_lost_share:
  MEA deforestation 0.0016797 -> 0.0001070   MEA wildfire  0.0000286 -> 0.0006953
  EUR/NEU shifting  6.8e-05   -> 0           SSA shifting  0.0036349 -> 0.0035955

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FQU9MetaTSbqRJVrCypV9n
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QBJaUn8DLgzgyZUc8CRunM
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FQU9MetaTSbqRJVrCypV9n
checkCurtisTable1() sat between the roxygen block and the function it documents, so
roxygen attached the block to the helper. Helper moved above the block; man/ regenerated.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FQU9MetaTSbqRJVrCypV9n
New madrat source: GFW data-api table gadm__tcl__iso_change, UMD/Hansen annual tree
cover loss crossed with the Sims et al. (2025) 1 km driver map. Country level,
2001-2025, eight classes (the paper's seven plus Unknown, which the data carries).
download: pinned immutable dataset version, 30% canopy threshold, open /download/csv
endpoint, no credential. read: checks schema, class set, single threshold, and that
the driver file sums to a separately queried totals file. convert: XKO/ZNC/XAD folded
onto SRB/CYP, three GADM placeholders dropped (0.031% of loss), 0.5% drop tolerance.
Tests are synthesized known-bug cases. Driver shares match Sims table 3 within 3.5 pp
except Africa and Oceania: the cross-tab uses the annually updated map (v1.13,
2001-2025), not the paper's.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FQU9MetaTSbqRJVrCypV9n
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QBJaUn8DLgzgyZUc8CRunM
…tFireLoss

Both gain `source` ("GFW" default, "Curtis") and `period` (years averaged for GFW,
default 2015:2024; Curtis is a single 2001-2015 mean). `period` rather than `years`
because calcOutput() consumes a `years` argument itself. Denominator (FRA 2020
natural forest 2010) and the `overall` FRA fire column are unchanged. source =
"Curtis" reproduces the previous output exactly (max difference 0).
Note: fullMAGPIE now writes nine columns, which module 35's six-element
driver_source set rejects at compile time until the model side is updated.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FQU9MetaTSbqRJVrCypV9n
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QBJaUn8DLgzgyZUc8CRunM
…enously

calcForestFireLoss(source = "GFW") returns shifting cultivation, wildfire and other
natural disturbances only. Module 35 resets age classes with regrowth rather than
removing forest, so a class belongs here only if it leaves the land as forest and
MAgPIE does not already decide it: permanent agriculture, hard commodities, logging,
settlements and unknown are excluded and published by
mrvalidation::calcValidTreeCoverLoss instead. readGFWLossByDriver() still returns all
eight classes.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FQU9MetaTSbqRJVrCypV9n
Wildfire and other natural disturbances leave gfwModelDrivers. The area this parameter
moves is booked as land-use-change CO2 by module 52 and drives DEFO_largeFires in
magpie4::reportFireEmissions, so a class must also be a land-use process. Wildfire is
additionally already in the fire-enabled LPJmL run behind the natural-vegetation
carbon densities. Measured before removal: wildfire 8.66 Mha/yr (72.5% REF+CAZ),
other natural 0.45, shifting cultivation 3.17. overall and shifting_cultivation are
bit-identical before and after in all 12 regions. Both dropped classes stay in
calcValidTreeCoverLoss.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014XmKyZUQSGPk6rJna5eD62
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QBJaUn8DLgzgyZUc8CRunM
Validation key, Date, CITATION.cff and README refreshed. Version stays 0.76.0 (bumped
earlier on this branch, unpublished). Re-run immediately before merge: validkey and
isVersionUpdated go stale on their own.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014XmKyZUQSGPk6rJna5eD62
The year-gap check cannot fire on a truncated download (a tail cut leaves no gap
in the series) and the country-count threshold only fires on truncations that the
coverage check against loss_totals.csv already catches. Both removed with their
tests; the coverage test now cuts the tail of the country list.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GaYsqSd2gZdixtjH8XWu2k
…IE parameter

fullMAGPIE wrote f35_forest_disturbance_share.cs4 for module 35, where the parameter
has been declared and included since December 2020 and read by no equation in any
revision. The model side removes the declaration; nothing else calls the function.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GaYsqSd2gZdixtjH8XWu2k
mapping.csv in inst/extdata is now comma-separated with quoted names (sixteen names
contain commas); the reader detects the separator so a shared source folder still
holding the semicolon original keeps working. Four over-long lines wrapped.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GaYsqSd2gZdixtjH8XWu2k
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QBJaUn8DLgzgyZUc8CRunM
…share

The share was UMD tree cover loss over FRA 2020 naturally regenerating forest area,
two products on different bases. downloadGFWLossByDriver now also fetches UMD tree
cover extent in 2000 at the same 30% threshold from gadm__tcl__iso_summary
(extent_2000.csv); readGFWLossByDriver(subtype = "extent") returns it after checking
that every country with loss has extent and that cumulative 2001-2025 loss never
exceeds it (max 0.998 on the real data). calcForestLossShare divides GFW loss by
that extent; the Curtis source keeps FRA and reproduces the previous output bit for
bit. The silent clamp of shares above 1 now warns for driver columns; overall (FRA
fire area) exceeds 1 in a few savanna countries and is never read by GAMS.
Numerator unchanged; the shifting_cultivation share moves by region: SSA x1.09,
LAM x0.91, OAS x0.72, IND x1.74 (world denominator 3834 -> 3986 Mha).

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GaYsqSd2gZdixtjH8XWu2k
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QBJaUn8DLgzgyZUc8CRunM
@mscrawford

mscrawford commented Sep 14, 2026 •

Copy link
Copy Markdown
Contributor Author

To @codeZeilen and @pascal-sauer: This has a lot of safety guards on this older dataset. I kept them in, but at the same time am skeptical about this being worth the extra code. Should I remove the checks?

The function has returned forest area lost per year by driver since the Curtis table
was bound into it (51a8a88, 2020); the FRA fire series it started as is now the
overall column. calcForestLossShare is its only caller. No output change:
f35_forest_lost_share is identical for both sources.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QBJaUn8DLgzgyZUc8CRunM
mscrawford added a commit to mscrawford/magpie that referenced this pull request Sep 14, 2026
mrland's calcForestFireLoss is now calcForestLossByDriver (pik-piam/mrland#78).
Comment only.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QBJaUn8DLgzgyZUc8CRunM
Comment thread inst/extdata/ForestLossDrivers/SOURCE.md Outdated
Comment thread R/fullMAGPIE.R

# 35 natural vegetation
calcOutput("ForestLossShare", round = 7, outputStatistics = stats, file = "f35_forest_lost_share.cs3")
calcOutput("ForestDisturbances", round = 7, outputStatistics = stats, file = "f35_forest_disturbance_share.cs4")

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing that came up in the process of making this bugfix is that Michael Windisch's old code on forest disturbances was removed from MAgPIE but not removed from the input data that goes into MAgPIE.

Comment thread tests/testthat/test-GFWLossByDriver.R Outdated
`{d}` and `{v}` are Rd markup in non-markdown roxygen; R CMD check noted "Lost braces"
at downloadGFWLossByDriver.Rd:17. Now `<dataset>` and `<version>`.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QBJaUn8DLgzgyZUc8CRunM
Comment thread inst/extdata/ForestLossDrivers/SOURCE.md Outdated
Comment thread inst/extdata/ForestLossDrivers/SOURCE.md Outdated
Comment thread inst/extdata/ForestLossDrivers/SOURCE.md Outdated
Comment thread inst/extdata/ForestLossDrivers/SOURCE.md Outdated
Comment thread inst/extdata/ForestLossDrivers/SOURCE.md Outdated
Comment thread inst/extdata/ForestLossDrivers/SOURCE.md Outdated
Comment thread R/readForestLossDrivers.R Outdated
Comment thread R/calcForestLossByDriver.R Outdated
Comment thread R/convertGFWLossByDriver.R Outdated
@pascal-sauer

Copy link
Copy Markdown
Contributor

Seems like you're talking to claude and not reviewers, so I removed reviewers again for now, so they don't get notified each time.

mscrawford and others added 5 commits September 22, 2026 13:34
… file

Remove inst/extdata/ForestLossDrivers/SOURCE.md, checkCurtisTable1() and
tests/testthat/test-GFWLossByDriver.R, move the GFW constants into the
function bodies and shorten the comments in the ForestLossDrivers and
GFWLossByDriver functions. Rd files regenerated.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
README.md taken from master; buildLibrary regenerates it.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
readForestLossDrivers spread each regional total over countries by FRA 2015
forest area, while calcForestLossShare divides by and aggregates with FRA 2010.
That rescaled every country by its own 2015/2010 area ratio, manufacturing up
to 2.4x variation inside a source region that carries a single rate. Aligned
to 2010.

Also records why Table 1's rows are normalised to sum to 100, since the printed
rows sum to 99-102 and the drivers must not exceed the observed loss.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
readGFWLossByDriver.R held three top-level objects where the package's other
read functions hold one. The two check helpers and the class map are now local
to the function.

Guards go from eleven to nine. Dropped: the pinned global-loss total, which
sits at +-5 per cent, while a re-pinned version carrying one more year moves
it by 4 per cent - it could not catch the drift it was there for. Merged: the
two single-file threshold checks become one assertion that the extent file
carries the same canopy threshold as the loss file, which is the invariant the
share depends on; that the value is the pinned one is checked in the download,
where it is pinned.

Behaviour unchanged: every regional share is identical, and a fresh run of the
chain reproduces the input files byte for byte outside their provenance header.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@mscrawford mscrawford added bug Something isn't working enhancement New feature or request AI labels Sep 23, 2026

@flohump flohump left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @mscrawford, both the Curtis fix and the switch to GFW make sense.

Review produced with the help of Claude.

Suggestions:

  1. Merge together with magpie#934 and the new input data revision, since the rename breaks develop.
  2. Keep Curtis as a sensitivity source, but make it usable: it is a hand-transcribed table shipped in inst/extdata, so read it from there directly and drop downloadForestLossDrivers() (the HPC source folder still holds the uncorrected 2020 forest_loss.csv and mapping.csv and is never re-staged); and in the Curtis branch rename shifting_agriculture to shifting_cultivation and keep only that column, as the GFW branch does, so magpie#934 can read it.
  3. Denominator: MAgPIE applies the rate to natural forest, but it is divided by tree cover extent (incl. plantations). By region this barely matters for SSA/LAM, but OAS gets 0.00175 instead of 0.00243 and IND 0.00287 instead of 0.00165 (FRA naturally regenerating forest as denominator). I'd divide by FRA naturally regenerating forest, as the Curtis branch does.
  4. Decomposition run in magpie#934 with fixed Curtis, to separate the bug fix (SSA) from the source switch to GFW.

@mscrawford

Copy link
Copy Markdown
Contributor Author

Thanks @mscrawford, both the Curtis fix and the switch to GFW make sense.

Review produced with the help of Claude.

Suggestions:

1. **Merge together** with magpie#934 and the new input data revision, since the rename breaks develop.

2. **Keep Curtis as a sensitivity source, but make it usable:** it is a hand-transcribed table shipped in `inst/extdata`, so read it from there directly and drop `downloadForestLossDrivers()` (the HPC source folder still holds the uncorrected 2020 `forest_loss.csv` and `mapping.csv` and is never re-staged); and in the Curtis branch rename `shifting_agriculture` to `shifting_cultivation` and keep only that column, as the GFW branch does, so magpie#934 can read it.

3. **Denominator:** MAgPIE applies the rate to natural forest, but it is divided by tree cover extent (incl. plantations). By region this barely matters for SSA/LAM, but OAS gets 0.00175 instead of 0.00243 and IND 0.00287 instead of 0.00165 (FRA naturally regenerating forest as denominator). I'd divide by FRA naturally regenerating forest, as the Curtis branch does.

4. **Decomposition run** in magpie#934 with fixed Curtis, to separate the bug fix (SSA) from the source switch to GFW.

Hi Florian, thanks for the review. A few very quick thoughts with follow-up later.

  1. Absolutely, the order will be mrland + magpiesets -> mrvalidation -> magpiemodel.
  2. The download function is actually just reading from extdata, where the table is stored. I like that because it's keeping the metadata in a nice way, and would defend this judgement call. Happy to discuss.
  3. That's a good idea -- thanks.
  4. I already have these runs complete. I will update the magpiemodel PR hopefully soon.

Comment thread R/downloadForestLossDrivers.R Outdated
stop("ForestLossDrivers package data not found. Is mrland installed correctly?")
}

staged <- file.copy(list.files(extdata, full.names = TRUE), ".", overwrite = TRUE)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
staged <- file.copy(list.files(extdata, full.names = TRUE), ".", overwrite = TRUE)
staged <- file.copy(list.files(extdata, full.names = TRUE), ".", overwrite = TRUE, copy.mode = FALSE)

Packages and all their files (including extdata) in the renv cache are write protected. copy.mode = FALSE ensures write protection is dropped when copying (makes cleanup easier, we only want to protect renv cache).

Comment thread R/downloadForestLossDrivers.R Outdated
Comment on lines +18 to +21
if (extdata == "") {
stop("ForestLossDrivers package data not found. Is mrland installed correctly?")
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (extdata == "") {
stop("ForestLossDrivers package data not found. Is mrland installed correctly?")
}

Handling this case is probably not worth it I'd say. In general LLMs try to cover every single angle that something can go wrong, but that is often unnecessary in my opinion.

Comment thread R/downloadForestLossDrivers.R Outdated
#' \dontrun{
#' madrat::downloadSource("ForestLossDrivers")
#' }
downloadForestLossDrivers <- function() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tscheypidi what's your take on this pattern, is this idiomatic madrat? download function pulling from extdata?

@pascal-sauer pascal-sauer Sep 28, 2026 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One problem we definitely have with this: the download function is only called once. If the data in extdata is updated this update will not be applied until then source folder is manually removed or until the download function is run manually.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, crap. I hadn't considered that... What do you think the best way forward is here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On second thought though: This is the Curtis data, taken from a table of a particular paper. Therefore, the data in extdata will never change. Granted, it was bugged once, and this doesn't necessarily help in the generalized case. So I'm happy to move the metadata into a readSource function, e.g.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Solved differently: Curtis is now a plain readSource() from the source folder, with no download function and no copy in the package, so there is nothing to go stale. I'll replace the file in the shared source folder with the merge.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correction: no file in the shared source folder is replaced. The corrected table is a new file there, forest_loss_fixed.csv, read since ce477f3; forest_loss.csv and mapping.csv stay untouched for older mrland versions.

Comment thread R/downloadForestLossDrivers.R Outdated
person(c("Christy", "M."), "Slay"),
person(c("Nancy", "L."), "Harris"),
person("Alexandra", "Tyukavina"),
person(c("Matthew", "C."), "Hansen")),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

duplicated author list, but that's ok

Comment thread R/calcForestLossByDriver.R Outdated
#' @description Forest area lost per year by driver, in Mha, from the Global Forest Watch driver
#' product of Sims et al. (2025) (\code{source = "GFW"}, default) or from Table 1 of Curtis et al.
#' (2018) (\code{source = "Curtis"}), together with the FRA 2020 forest fire area as
#' \code{overall}.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we have a data product combined from 3 sources is my understanding. Does it make sense to provide a little more detail how they are combined?

Comment thread R/calcForestLossShare.R Outdated
#' @author Abhijeet Mishra, Michael Crawford
#' @importFrom magclass setYears setNames getItems
#' @importFrom utils head
#' @importFrom madrat calcOutput readSource

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#' @importFrom madrat calcOutput readSource

Comment thread R/calcForestLossShare.R Outdated

lost_area <- calcOutput("ForestFireLoss",aggregate=FALSE) # Convert=T returns Mha area lost to fire
forest_area <- setYears(setNames(readSource("FRA2020",subtype = "forest_area",convert = TRUE)[,"y2010","naturallyRegeneratingForest"],NULL),NULL) # Convert=T returns Mha area of forests
calcForestLossShare <- function(source = "GFW", period = 2015:2024) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
calcForestLossShare <- function(source = "GFW", period = 2015:2024) {
calcForestLossShare <- function(src = "GFW", period = 2015:2024) {

Renaming gets around the linter issues

Comment thread R/convertGFWLossByDriver.R Outdated
Comment on lines +13 to +14
#' @importFrom madrat toolCountryFill
#' @importFrom magclass getItems

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#' @importFrom madrat toolCountryFill
#' @importFrom magclass getItems

Comment thread R/downloadGFWLossByDriver.R Outdated
Comment on lines +107 to +108
"of",
"Sims et al. (2025) plus 'Unknown'. NOTE: the driver map underlying",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"of",
"Sims et al. (2025) plus 'Unknown'. NOTE: the driver map underlying",
"of Sims et al. (2025) plus 'Unknown'. NOTE: the driver map underlying",

Comment thread R/readGFWLossByDriver.R Outdated
#' @return magpie object; loss by ISO country x year (2001-2025) x driver class, extent by ISO
#' country for y2000. Mha.
#' @author Michael Crawford
#' @importFrom magclass as.magpie magpiesort

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#' @importFrom magclass as.magpie magpiesort

mscrawford and others added 3 commits September 29, 2026 15:58
- Curtis Table 1 is read from the source folder; the country mapping moves to
  inst/extdata/regional; downloadForestLossDrivers is removed
- GFW shares are divided by FRA 2020 naturally regenerating forest; the extent
  download is removed
- only shifting_cultivation is returned (overall dropped); source -> src

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@mscrawford

Copy link
Copy Markdown
Contributor Author

Thanks, all four are in 6f8b408.

  1. Order as agreed: mrland and magpiesets, then mrvalidation, then magpie#934 with the new input revision.
  2. Only shifting_cultivation now, from both sources. I kept Curtis a normal madrat source rather than reading inst/extdata.
  3. Done: the share is divided by FRA 2020 naturally regenerating forest (2010 for Curtis), so the tree cover extent download is removed.
  4. I'll update magpie#934 with the decomposition once the new inputs are built.

@mscrawford

Copy link
Copy Markdown
Contributor Author

Thanks, all your suggestions are in 6f8b408. Beyond them, the tree cover extent download is removed, since Florian's point 3 made it unused.

mscrawford and others added 2 commits October 1, 2026 13:45
The 2020 forest_loss.csv stays in the source folder for older mrland versions.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

This branch has not been deployed

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

Labels

AI bug Something isn't working enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants