Test covariates by jackknife, and fit ensembles of model types - #2
Merged
Conversation
YAML 1.1, which is what yaml::read_yaml() parses, reads a bare `n` as the
boolean false - along with y, yes, no, on, and off. That is right for a
value and wrong for a key, and the difference is silent. A derivoce step
written
- type: lag_covariate
vars: [CHL]
n: 2
parses to a list whose key is named FALSE, so spec$n is NULL and
R/derivoce.R falls back to a one-month lag. The config asked for two, the
run used one, and nothing said so. cfin_gom.yaml hid it by asking for the
value the fallback already gives.
yaml's handlers are given the scalar as it was written - "n", not FALSE -
so the source text is recoverable. It cannot be resolved while parsing,
though, because a handler cannot see whether it is being called for a key
or for a value. So this marks each boolean scalar with its source text and
then walks the finished structure: text in a name position is the key the
file wrote, text in a value position becomes the logical it meant.
The marker is a string prefix rather than an attribute or a class because
it has to survive yaml collapsing a sequence of scalars into an atomic
vector, which drops attributes. [true, false] would otherwise come back as
two strings.
Only the read side was affected. yaml::as.yaml() already quotes these keys,
so a config generate_config() or save_config() wrote has always been read
correctly by anything. cfin_gom.yaml and the README example are quoted
anyway, for readers that are not this package.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two things a run can now do that it could not, both off by default and both driven from the config. covariates.jackknife refits the model without each covariate in turn, over the same cross-validation folds as the main fit, and reports how much worse it ranks stations. Alongside it goes the other half of the classical jackknife, the model on that covariate alone, because the two answer different questions: leaving one out measures what only that covariate carries, fitting it alone measures what it carries at all. A covariate can score high on one and nothing on the other, and that is the case worth seeing - information that is real and duplicated is not the same thing as information that is not there. The reported p-value is a one-sided test of the per-fold differences with the variance correction of Nadeau and Bengio (2003). The correction is the load-bearing part. A plain paired t-test treats the folds as independent when any two training sets share most of their rows, so its variance estimate is badly optimistic and it finds significance everywhere (Dietterich 1998). There is no unbiased estimator of the variance of k-fold cross-validation (Bengio and Grandvalet 2004), so this inflates the naive variance by 1/k + 1/(k-1) instead, which roughly halves the statistic. For a GLM and a GAM the classical test is reported beside it rather than instead of it - drop-in-deviance for the first, mgcv's approximate term p-value for the second. That is why the fold test is the default criterion: it means the same thing for all four model types, and a forest has no likelihood to take a ratio of. Dropping is opt-in and defaults to off. A covariate that fails this test is one the other covariates already account for on these stations, which says as much about collinearity in this sample as about ecology - depth and surface temperature carry much of the same information on a shelf, and the test will call either one redundant depending on which the model reached for first. So the default is a table and a message saying what dropping would have removed. What it does remove goes through covariates.exclude, the mechanism that already existed, so a dropped covariate is still fetched for anything that needs it as an ingredient. model.ensemble fits several model types on the same data and combines them, which is BIOMOD_EnsembleModeling() from the pipeline this package replaces. Four combination rules, all computed and written; the config picks which one becomes the suitability layer. The ensemble's evaluation is its own rather than the average of its members'. Every member is fitted on the same folds from the same seed, so their held-out predictions line up by .row, and the combined out-of-fold predictions go through the same evaluation_table(), optimal_threshold() and bootstrap_evaluation() a single model does. Averaging member scores would have reported the ensemble as the average of its parts, which is not what an ensemble does: combining members that make different mistakes beats all of them and combining members that make the same mistakes does not, and only a cross-validated ensemble prediction tells those apart. The word ensemble was already taken. projection.uncertainty combines over resamples of the data within one algorithm; this combines over algorithms. Both can be on, and a projection then carries algorithm_sd beside suitability_sd rather than one standing in for the other. uncertainty_layers() becomes projection_layers() for the same reason. Both features parallelize over model fits through one shared helper, which forks - so Windows runs sequentially and says so rather than pretending. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two new sections in the vignette, both running on the mock data the rest of it uses, and worked examples in the README beside the config blocks that were already there. The vignette sections say what the mock run actually shows rather than what would have been convenient. No covariate reaches significance on three covariates over a few hundred synthetic stations at five folds, and two contributions come out negative - so the text says that this is the correct answer rather than a broken test, and that a method returning confident answers from this much data would be the one to distrust. The `jday` row is the good illustration of the point the two halves exist to make: it scores well on its own and contributes nothing on top of the others, which is information that is real and duplicated rather than information that is not there. The ensemble section needed no such hedging. On this run the combined model beats both of its members out of fold, and the per-member importance columns show the forest and the GLM disagreeing sharply about one covariate, which is exactly what those columns are kept for. jackknife_dropped() now defaults its settings argument to the ones the result carries, so asking a jackknife what it would drop takes only the result. The alternative in a vignette was reaching for an internal. Version goes to 0.2.0. Two features, no exported function changed meaning; uncertainty_layers() became projection_layers() but was never exported. inst/CITATION reads the version from DESCRIPTION, so it needs no edit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
bdb47bf moved the citation check onto the shared engine in chross22/distsamp and deleted inst/tools/check_citations.R. tests/testthat/test-citations.R stayed. Every one of its seven tests calls a helper that sources that script and skips when it is absent, so since that commit all seven have skipped on every run - and a skipped test reads as a passing test in most summaries. The file was worth having for a reason it no longer serves. Its comment says the bug worth guarding against is the checker passing everything, "a matcher that is too loose reports a healthy repo whatever the state of the references, which is worse than not having the check at all". That is true, and it is now distsamp's matcher to guard. Deleted rather than moved. Nothing in the file is about taupatch: all seven exercise the retired script's internals through `sys.source()`, so there is nothing here to port. distsamp already covers two of the seven in tests/testthat/test-citation-engine.R - the DOI styles and the trailing punctuation - and whether the other five still describe how the shared engine behaves is a question about that engine, not about this repo. test-citation.R, singular, is untouched. It tests inst/CITATION and the README's citation section, is offline, and has been passing throughout. Also adds the empty-config-field case to the YAML boolean tests. A concurrent review of the same fix raised it: the walk rebuilds every list it descends into, and rebuilding by assigning back - `x[] <- lapply(x, ...)` - drops elements whose value is NULL, which is what an empty YAML field parses to. This implementation replaces the list instead and was never affected, but the hazard is real enough for the next person to deserve a test rather than a comment. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The tests already here establish that `n: 2` survives the parse as a field named n. This asserts the consequence: that the step derivoce then runs is a two-step lag, with August carrying June's value and the first two months undefined rather than the first. That is the failure the bug actually produced. A config could parse correctly and still be wired to a reader that never asked for the field, and nothing above this test would notice. The config has to come off disk for either to mean anything. Built as an R list, `n` is an ordinary name and the test passes against a parser that loses it - so mock_config_yaml() splices the derivoce block into the shipped mock config and loads the result, which is the only path that goes through yaml::read_yaml(). Verified it bites: with load_config() pointed back at a bare read_yaml(), this fails three assertions and then errors with "step 'lag_covariate' has no argument(s) FALSE", which is the bug stating its own name. Ported from a concurrent session that fixed the same bug independently. Its own implementation is superseded by 354ceb7, which additionally keeps a sequence of bare booleans - [yes, no] - as logicals rather than strings; this test is the part of that work worth keeping. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The citation check failed on this branch: six DOIs cited but absent from tools/citations.csv. The shared engine requires every DOI in the docs to be in the registry, and the registry is not generated - a new reference is a new row. Five of the six are now rows, with the fields taken from CrossRef rather than typed, since those are the fields the engine compares against CrossRef: araujo2007 ensemble forecasting - why an ensemble of algorithms marmion2009 the combination rules, compared elith2011 the leave-one-out / only-one jackknife pair nadeau2003 the variance correction dietterich1998 the Type I error of the uncorrected test Two details. The registry `year` is the issue year the docs cite, not CrossRef's, which is a year earlier for the two published online first - the engine allows exactly that gap. And `first_author` is written unaccented, Araujo, because the comparison strips every non-ASCII character to a space on both sides: an accented registry entry would normalise to "ara jo" and never match CrossRef's "araujo". The sixth, Bouckaert and Frank, keeps its reference but loses its DOI link. It is a Springer chapter, so its identifier ends _3, and the engine's DOI pattern treats an underscore as a terminator - reasonable in Markdown, where _ is emphasis, but it means that DOI cannot be written down anywhere the checker reads. Registering the truncated form would have made CI pass while recording the book's identifier for a chapter citation, so the reference now gives the volume and page range instead and says why in a note. Verified by running the shared engine against this working tree rather than by pushing and waiting: all five sections pass. That is also how the failure should have been caught the first time. The local script this branch retired only resolved DOIs and had no concept of a registry, so "every citation resolves" was true and beside the point. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
R CMD check --as-cran sets _R_CHECK_LIMIT_CORES_, and under it parallel::mclapply() does not quietly use fewer cores: parallel:::.check_ncores() stops with "3 simultaneous processes spawned" the moment more than two are asked for. The default here was one fewer than the machine's physical cores, so on any runner with four the jackknife errored - taking six tests and the vignette down with it. Windows and macOS passed the same commit, which is the part worth noting. Windows never forks, so resolve_workers() had already returned 1 there, and macOS reported few enough cores to stay under the limit. Only Linux had enough cores to trip it, so this looked like a platform bug and was a default that is wrong everywhere and visible in one place. core_ceiling() now reports what the session permits, and resolve_workers() takes the minimum of the request, the task count and that ceiling. It caps an explicitly configured count too: the limit is not a preference, and mclapply refuses rather than clamping, so honouring `workers: 8` under a check would only reproduce the error the user was trying to avoid. Two workers compute the same answer as eight. options(mc.cores) now sets the default as well, ahead of detectCores(). It is the knob R users already reach for, and a package that ignores it makes them learn a second one. Verified under _R_CHECK_LIMIT_CORES_=TRUE, where the six tests that failed on CI now pass, and with R CMD check --as-cran, where the vignette that failed to re-build now does. That flag is the one my earlier local check was missing: without it the variable is never set and the whole failure mode is invisible. withr moves to Suggests, for the tests that set the variable and restore it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The assertion was that resolve_workers(NULL, ...) returns the two workers options(mc.cores) asked for. On Windows it returns one, because there is no forking there and resolve_workers() says so a few lines earlier - so the test described the platforms it happened to be written on and failed on the one it did not. Windows was the only green Linux-and-macOS-failing job on the previous commit and the only red one on this, for the same underlying reason in reverse: worker counts differ by platform, and an expectation on an exact count has to say which platform it means. The other assertions added alongside it are inequalities - at most two under a core-limited check, at most the task count, at least one - and every one of them holds at a worker count of one, so this was the only one that needed it. Checked rather than assumed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The marker carrying a boolean's source text through parsing was built out of control characters, on the reasoning that nothing in a YAML file could collide with them. Nothing can. But two SOH bytes in a string literal are enough for file(1) to classify the whole of R/config.R as `data` rather than as source, and anything that asks the same question - an editor, a diff viewer, a blob preview - then presents a 700-line file as binary. It reads as a corrupted file, which is a poor trade for a collision that was never going to happen. The marker is now <taupatch:yaml-bool>, in plain ASCII, and the file is UTF-8 text again. Nothing is given up. Only the two boolean handlers ever prepend the marker, and `yaml` only ever hands them scalars it has itself resolved as booleans - a quoted "true" carries no boolean tag and is never marked, which is already covered by a test. The one way left to collide is a config value that genuinely begins with the string <taupatch:yaml-bool>. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two things the README left a reader to work out.
install_github() does not build vignettes unless asked - build_vignettes
defaults to FALSE - so vignette("taupatch") finds nothing after following the
install instructions exactly. Nothing is broken and nothing says so. The
install section now gives the flag, warns that building runs the pipeline as
it renders, and links the source for anyone who would rather just read it.
The species section explained what a catalog looks like without explaining
what decides its shape. It showed column_prefix throughout and never
abundance_column, so the two forms read as a style choice rather than as an
answer to "does this database resolve life stages for this taxon?". There is
now a table for the two forms and what each lets a run do, and the reason
column_prefix needs stage columns to match against: it matches
<prefix>_<something>, which is also why `stages` alongside abundance_column is
an error.
It also never mentioned species_catalog_from(), which reads a database and
picks the right form per taxon so nobody has to. Its output is now shown for a
header with one staged taxon and one total, since seeing the two entries side
by side is the explanation. And a short table separates it from zoop_taxa()
and available_species(), which answer neighbouring questions and are easy to
reach for by mistake.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
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.
Two features, both off by default and both driven from the config, plus a YAML bug found on the way.
Testing covariates by jackknife
covariates.jackkniferefits the model without each covariate in turn, over the same cross-validation folds as the main fit, and reports how much worse it ranks stations. Alongside it goes the other half of the classical jackknife — the model on that covariate alone — because the two answer different questions:score_withoutscore_onlyA covariate can score high on one and nothing on the other. That combination — information that is real and duplicated — is a very different situation from information that is not there, and ranking on contribution alone treats them identically.
The significance test
Two, reported side by side:
p_value— one-sided on the per-fold differences, with the variance correction of Nadeau & Bengio (2003). The correction is the load-bearing part: a plain paired t-test treats the folds as independent when any two training sets share most of their rows, so it finds significance almost everywhere (Dietterich 1998). There is no unbiased estimator of the variance of k-fold CV (Bengio & Grandvalet 2004), so this inflates the naive variance by1/k + 1/(k-1)instead.p_adjustedis Holm across covariates.parametric_p— drop-in-deviance LRT forglm,mgcv's approximate term p-value forgam,NAforrf/brt. Which is why the fold test is the default criterion: it means the same thing for all four model types.Dropping is opt-in
dropdefaults to false. The run reports the table and says what dropping would have removed. A covariate that fails this test is one the other covariates already account for on these stations, which is a statement about collinearity in this sample at least as much as about ecology — depth and surface temperature carry much of the same information on a shelf, and the test will call either one redundant depending on which the model reached for first.keepprotects a covariate that is in the model because the study is about it;min_predictorsis a floor.What it does remove goes through the existing
covariates.exclude, so a dropped covariate is still fetched for anything that needs it as an ingredient.Ensembles of model types
model.type: ensemblefits several algorithms on the same data and combines them —BIOMOD_EnsembleModeling()from the pipeline this package replaces. Four rules (mean,weighted_mean,median,committee), all computed and written; the config picks which becomes thesuitabilitylayer. The result is a drop-in for afit_patch_model()object, soproject_patch_model()projects it unchanged.The evaluation is the ensemble's own, not the average of its members'. Every member is fitted on the same folds from the same seed, so held-out predictions line up by
.row, and the combined out-of-fold predictions go through the sameevaluation_table()/optimal_threshold()/bootstrap_evaluation()a single model does. Averaging member scores would report the ensemble as the average of its parts, which is not what an ensemble does — combining members that make different mistakes beats all of them, combining members that make the same mistakes does not, and only a cross-validated ensemble prediction tells those apart. On the vignette's mock run the ensemble reaches 0.903 out of fold against members at 0.892 and 0.901.Two different things are called an ensemble now, and they are independent:
model.ensemblealgorithm_sdprojection.uncertaintysuitability_sdBoth can be on; each member carries its own resample interval, those replicates are pooled by member weight, and algorithm disagreement is reported on top.
uncertainty_layers()is renamedprojection_layers()for the same reason (internal, never exported).The YAML bug
Found while testing. YAML 1.1 — what
yaml::read_yaml()parses — reads a barenas the boolean false, along withy,yes,no,on,off. Right for a value, wrong for a key, and silent: a derivoce step writtenparses to a list whose key is named
FALSE, sospec$nisNULLandR/derivoce.Rfalls back to a one-month lag. The config asked for two, the run used one, nothing said so.cfin_gom.yamlhid it by asking for the value the fallback already gives.yaml's handlers receive the scalar as it was written, so the source text is recoverable — but a handler cannot see whether it is being called for a key or a value, so this marks each boolean scalar and then walks the finished structure: text in a name position is the key the file wrote, text in a value position becomes the logical it meant. The marker is a string prefix rather than an attribute, because it has to surviveyamlcollapsing a sequence of scalars into an atomic vector.Only the read side was affected —
yaml::as.yaml()already quotes these keys, so a configsave_config()wrote has always been read correctly by anything.Parallelism
Both features spread their model fits over one shared helper, which forks — so Windows runs sequentially and says so rather than pretending. It is one fit per covariate per fold either way.
Verification
R CMD check --no-manual: Status: OK — 0 errors, 0 warnings, 0 notes, with the vignette built and the suite run inside it.test-jackknife.R,test-ensemble.R, and the YAML cases intest-config.R.algorithm_sd,suitability_sd, andnoveltyas distinct layers.Version bumped to 0.2.0. No exported function changed meaning.
Retiring a stale test file
tests/testthat/test-citations.R(7 tests) testedinst/tools/check_citations.R, which bdb47bf retired in favour of the shared engine inchross22/distsamp. All 7 have skipped silently on every run since — and a skipped test reads as a passing test in most summaries. Deleted rather than moved: every one exercises the retired script's internals throughsys.source(), so there is nothing taupatch-specific to port.distsamp'stest-citation-engine.Ralready covers 2 of the 7 (the DOI styles and the trailing punctuation). The other 5 have no visible counterpart there — doi.org de-duplication, the two year-extraction cases, live-vs-dead handle, and which files get scanned. Whether those still describe how the shared engine behaves is a question about that engine rather than this repo, so I have not toucheddistsamp. Worth a look, and the other migrated repos likely carry the same stale file.test-citation.R(singular) is untouched — it testsinst/CITATION, is offline, and has been passing throughout.The suite now runs with 1122 passing and zero skips.
🤖 Generated with Claude Code