Fix HAP model bias correction, redesign by_gr, and validate against real data - #6
Open
jonsampedro wants to merge 2 commits into
Open
Fix HAP model bias correction, redesign by_gr, and validate against real data#6jonsampedro wants to merge 2 commits into
jonsampedro wants to merge 2 commits into
Conversation
…eal data Model: - Fix zero-clipped health-impact predictions: the country-level bias correction can push a naive prediction negative, which was silently clamped to exactly 0 for ~35% of countries. A multiplicative correction (the textbook-correct form for a log-linear fixed-effects model) was tried and validated: it collapsed correlation with real GBD data from 0.86 to 0.18, because a common linear year trend that some countries (e.g. Uganda) don't follow makes a handful of implied fixed effects enormous, and multiplying amplifies that. Kept the additive form, now floored at 1% of the naive prediction instead of clamped to 0. - fit_model() now returns a reliability_ratio per country (bias correction size relative to the naive prediction), surfaced as a reliability flag in calc_hap_impacts() output. - Tested a GDP x PM2.5 interaction (RESET test flagged nonlinearity): significant and not a collinearity artifact, but the marginal PM2.5 effect is backwards from the ventilation hypothesis (negative in poor countries, positive in rich ones) -- not adopted. - calc_hap_impacts() accepts a pre-fit fit_result to avoid refitting the national regression on every scenario in a batch. - Ran full econometric diagnostics (serial correlation, cross-sectional dependence, heteroskedasticity, RESET, panel unit roots) confirming the Driscoll-Kraay SE choice and documenting known limitations. by_gr redesign: - by_gr = TRUE now reports within-country income-decile group SHARES of population, PM2.5+NOx exposure, and GDP (table + pie chart), instead of a fabricated group-level health-impact estimate produced by applying the country-level model (fit on between-country variation) to within-country inputs it was never validated against. Test suite: - Found and fixed several .RData snapshot tests (calc_hap_impacts, calc_ResidEm_grp x2, fit_model) that were vacuous: load() injects an object under its saved name as a side effect, which for these files matched the local variable holding the fresh test result, silently overwriting it before expect_equal() ran. Fixed via a load_snapshot() helper; replaced the brittle full-object fit_model snapshot with targeted structural/sign/parameter checks; regenerated the one legitimately stale snapshot. - Added coverage for by_gr = TRUE and the fit_result argument. Also: removed the calc_hia_adder()/hia_adder dataset (superseded by fit_model()'s internal bias correction), fixed a real bug where HIA_var = "yll" resolved to NULL, removed dead code left over from the by_gr redesign, gated by_gr-only GCAM queries behind by_gr for performance, and added a "Validating the econometric model" vignette documenting all of the above as a reproducible workflow. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Both the build and test_coverage workflows were failing for reasons
unrelated to the PR's code changes:
- build: remotes::install_deps() hit R's default ~60s internet
timeout downloading the large JGCRI/rmap and JGCRI/gcamdata GitHub
tarballs, consistently across multiple runs. Raised to 600s.
- test_coverage: the fs package (a transitive dependency via usethis)
failed to compile from source ("uv.h: No such file or directory")
because libuv1-dev wasn't installed on the runner. Added it, plus
the same timeout fix since it uses the same install_deps() step.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6 +/- ##
==========================================
+ Coverage 87.36% 96.78% +9.41%
==========================================
Files 7 6 -1
Lines 1171 1087 -84
==========================================
+ Hits 1023 1052 +29
+ Misses 148 35 -113 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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
1 participant
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.
Summary
by_grto report group shares (population/exposure/GDP) instead of a fabricated group-level health-impact estimate the model was never validated to produce.reliabilityflag tocalc_hap_impacts()output and afit_resultargument to avoid refitting the model across scenario batches..RDatasnapshot tests (aload()side-effect was silently overwriting the value being tested before the comparison ran).calc_hia_adder()/hia_adder(superseded), fixed aHIA_var = "yll"bug, removed dead code, gatedby_gr-only GCAM queries for performance.Test plan
by_grshares sum to ~1 and pie chart renders🤖 Generated with Claude Code