Take what fancyfx does better, and drop what it duplicates - #5
Open
chross22 wants to merge 5 commits into
Open
Conversation
fancyfx has gone 0.2.0 to 0.8.0 and now exports 26 functions, a good number of which overlap what taupatch built for itself. This is the survey: what to take, what to leave, what becomes redundant, and the two changes fancyfx needs first. The finding that shapes it is that fancyfx's four evaluation plots re-predict. They route through evaluation_pairs(), which calls predict_probability(model, newdata) and offers no way to hand it predictions that already exist - while taupatch's ROC, threshold, calibration and PR curves come from pooled out-of-fold predictions, each station scored by the fold model that did not see it. Handing fancyfx the final model and its own training stations would produce something weaker, and fancyfx says so itself: it warns, and captions the figure "In-sample: optimistic, and not validation". On a recipes workflow it does not even get that far, since the engine's response is the baked ..y rather than patch, and the call errors. So the four are not a swap as things stand. They become one with a small generalisation upstream - an entry point taking observed and predicted rather than a model - which fancyfx wants anyway, since it is what makes the package usable by anything that cross-validates rather than only by callers holding a single fitted model. Same for mess(): it duplicates novelty_surface() exactly, but takes only a SpatRaster and drops the novel_variable column that says which predictor put a cell outside the training range, which is the half worth having. What survives the "streamlining and improvement" test without any upstream work is the projection panels: plotUncertainty() and plotExtrapolation() take rasters, so none of the above applies, and they delete two hand-built ggplots while adding downsampling a real grid needs. Three are deliberately excluded rather than deferred. thin_points() changes which stations get modelled, which is a study's decision and not a package default. calc_deviance() is defined for glm and gam but not rf or brt, and taupatch reports metrics that mean the same thing across all four on purpose. plotHexbin() solves a problem the station map does not currently have. No code yet. The redundancy pass is deliberately scheduled last, against the code as it stands after the integration rather than against this document. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
plot_projection_uncertainty() was ninety lines of ggplot2 building two map panels and a diverging colour ramp, plus a helper whose whole job was to pin that ramp's break to the true zero. fancyfx draws both panels now, and all of it deletes: 101 lines out, 85 in, and the 85 are mostly the documentation of why the panels stay separate. It is a better picture as well as less code. plotUncertainty() and plotExtrapolation() downsample above max.cells, which a real Copernicus grid needs and the hand-built version did not do at all - it handed every cell to geom_raster and hoped. The extrapolation ramp diverges about zero in colours chosen to survive the common colour vision deficiencies, which the hand-rolled six-colour ramp was reaching for and did not achieve. Nothing about what is drawn changes. The panels are still separate rather than blended into one "confidence" layer, for the reason they always were: a cell can be stable across every ensemble member and still be extrapolated, and merging them averages that case away. The novelty subtitle still counts the cells outside the training range and names the covariate responsible, which is taupatch's own and not something fancyfx offers. Both panels take a single-layer raster, and the projection is a table of cells at that point, so projection_raster() changes the container. No resampling: project_patch_model() already builds its GeoTIFF from the same frame. fancyfx is a Suggests, so without it there is a message and no file, exactly as plot_gam_smooths() behaves. That is a real change for anyone who does not have it - the panels used to be drawn unconditionally - and it is the trade the deletion asks for. Worth revisiting whether fancyfx should simply be an Imports, since taupatch already takes datamatch from GitHub that way. Checked by drawing it: a mock ensemble run with projection.uncertainty on writes all three panels - resample spread, algorithm disagreement, novelty - stacked under one title, with the subtitle reporting 1 of 221 cells outside the training range and naming SSS. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two changes that only make sense together, because the first is what makes the second worth doing. fancyfx moves from Suggests to Imports. It was already in Remotes, so it was already being installed; what the Suggests bought was a set of branches for the case where it was not. Those branches were the cost: has_fancyfx() and its three call sites, an error in plot_gam_smooths(), a silent skip in write_effect_plots(), a message in plot_projection_uncertainty(), a reactive in the app, and two tests mocking an absence that can no longer happen. All of it goes. datamatch is already a hard dependency taken from GitHub the same way, so this is the arrangement the package already had for a dependency it cannot work without - and after the previous commit, the uncertainty panels cannot be drawn without this one. novelty_surface() now calls fancyfx::mess() rather than implementing MESS a second time. variable_similarity(), sixty lines of per-predictor percentile arithmetic, deletes. The two implementations agreed to the last decimal on every case tried, including which predictor gets named as the culprit, which is the argument for there being one of them. What does not change is the interface. novelty_surface() keeps its name, its arguments, and above all its column names: `novelty` and `novel_variable` are what the projection CSV and the GeoTIFF layers are written under, and a run's output should not lose them to an internal tidy-up. mess() returns `mess` and `mess_variable`; the renaming happens here, once. The behaviour is checked rather than assumed: an ordinary cell scores positive, a cell too warm or too cold names SST, one too green names CHL, a missing covariate gives NA, a grid with no shared predictors gives all NA, and a single-row grid keeps its shape - the case a vapply collapse used to get wrong. A cell where one predictor is NA and another is not still names the one that is worst among those it could score, exactly as before. The tests for variable_similarity() go with it. They tested arithmetic this package no longer owns, and fancyfx tests it. What replaces them tests what taupatch still asserts to its readers: that the scale reads 100 at the median, falls to zero at the edge of the training range, and goes negative outside it in proportion to how far. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two things taupatch could not say before, both from fancyfx. spatial_bias() is the spatial sorting bias of Hijmans (2012): how much closer the held-out patches sit to the training patches than the held-out non-patches do. It matters here more than in most places. ECOMON stations are not scattered at random - they sit on transects, revisited season after season - so a randomly held-out station usually has a near neighbour in the training folds, often the same station in another year. A model can then score well by recognising where it has already been rather than what makes a patch, and nothing in the ROC curve tells the two apart. It goes in evals.csv as an `ssb` row rather than into a file of its own, because it is the caveat on the roc_auc three rows above it and a reader who has to go looking for it will not. The run says it out loud too. The per-fold table is written to spatial_bias.csv, so one odd fold is visible rather than averaged into the answer. On the mock config it comes out at 0.2, which says that run's 0.85 AUC is optimistic - a real finding the evaluation could not previously produce, and an argument for spatially blocked folds rather than for a different model. This needed the fitted model to know where its stations were. model_data deliberately holds predictors only, and coordinates must not become predictors, so they are carried beside it and indexed by the same .row the held-out predictions use. projection_overlap() answers what compare_runs() cannot: not which surface scores better, but whether the two are the same surface. Schoener's D and Warren's I. Two runs can rank stations equally well and disagree completely about where the habitat is, and a comparison of AUCs reports them as equivalent. Cells are matched on coordinates rather than row order, and projections covering different ground are intersected with a warning, the same contract compare_runs() uses. Both statistics run high, and that is documented rather than left to be misread: two surfaces of independent noise over the same grid score about 0.7 on D and 0.9 on I, because both spread their probability over the same cells in similar proportions. Those are the floor for surfaces of this shape, not near-identity. niche_equivalency() is not wired up. It wants occurrence points for two entities and a refitting harness, which does not map onto a config-driven pipeline without inventing an interface for it, so it stays unused until there is a reason. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two gaps in the same function, both of which made a reasonable request unaskable through the app. Integrated chlorophyll without chlorophyll itself is an ordinary thing to want. The accumulated bloom is what feeds the animals sampled a month later; the instantaneous value at the moment of sampling is a different quantity and often not the interesting one. It could not be asked for. derivoce_choices() built its per-covariate options by looping over the *selected* covariates, so CHL_int appeared only once CHL had been made a predictor - and there was then no way to take CHL back out. The loop now runs over the whole catalogue, and the options that would cost a download say so in their group rather than their label, so the picker shows the cost without every entry carrying a parenthesis. The app already puts such ingredients in covariates.exclude, so they are fetched and not modelled, which is what that mechanism was built for. The second gap is the gradient of the current speed. UO and VO could each be differentiated and the speed could be computed, but the speed could not then be differentiated - and the gradient of the speed is the quantity a front in the flow actually is. Two components can each be changing steeply while the speed is constant, which is a turn rather than a shear. It is also the original pipeline's uv_grad, so the package could produce it from a hand-written config and the app could not. That needed a notion the choices did not have: a step that depends on another step rather than on a download. `depends` is that, and with_dependencies() follows it, so choosing speed_grad pulls current_speed in. Order needs no sorting - the choices already list a dependency before anything that reads it, and steps are emitted in that order. Every per-covariate step is offered on the speed, not only the gradient, and EKE gets a gradient too. A dependency is an ingredient in the same sense as a download, so derivoce_dependency_columns() names the derived columns a selection produced without anyone choosing them, and the app excludes those too. Picking speed_grad alone computes the speed and does not model it; picking both models both. Without this the speed would have arrived as a predictor nobody asked for, which is the bug the download side already avoided. 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
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.
Takes what fancyfx is now better at, deletes what it duplicates, and adds two evaluations taupatch could not do. Needs chross22/fancyfx#1, which is merged.
The plan, and what it excluded
docs/fancyfx_plan.mdsurveys all 26 fancyfx exports against what taupatch already had. The rule applied throughout: a duplicate is only redundant if the replacement answers the same question on the same evidence.Three things were excluded rather than deferred:
thin_points()— spatial thinning changes which stations get modelled. That is a study's decision, not a default a package should acquire.calc_deviance()— defined forglmandgambut notrforbrt. taupatch reports metrics that mean the same thing across all four on purpose.plotROC,plotThreshold,plotImportance,plotCalibration) — see below.Uncertainty panels drawn by fancyfx (−101 lines)
plot_projection_uncertainty()was ~90 lines of hand-builtggplot2plus a helper whose only job was pinning a colour ramp's break to zero. Both delete.Better as well as smaller:
plotUncertainty()/plotExtrapolation()downsample abovemax.cells, which a real Copernicus grid needs and the old version did not do at all — it handed every cell togeom_raster()and hoped. The diverging ramp is now colour-vision-safe, which the hand-rolled six-colour version was reaching for and did not achieve.Verified by drawing it: an ensemble run with uncertainty on produces all three panels — resample spread, algorithm disagreement, novelty — under one title, with the subtitle still counting cells outside the training range and naming the culprit. That subtitle is taupatch's own and is kept.
fancyfx becomes an Imports
It was already in
Remotes, so it was already being installed. What the Suggests bought was a set of branches for a case that could not really arise, and those were the cost:has_fancyfx()and three call sites, an error, a silent skip, a message, an app reactive, and two tests mocking an absence that can no longer happen.datamatchis already a hard GitHub dependency taken the same way.novelty_surface()delegates tomess()variable_similarity()— 60 lines of per-predictor percentile arithmetic — deletes. The two implementations agreed to the last decimal on every case tried, culprit variable included, which is the argument for having one of them.The interface is deliberately kept:
noveltyandnovel_variableare the names the projection CSV and GeoTIFF layers are written under, and a run's output should not lose them to an internal tidy-up.mess()returnsmess/mess_variable; the rename happens once, inside.Two new evaluations
spatial_bias()— the spatial sorting bias of Hijmans (2012). It matters here more than in most places: ECOMON stations sit on transects revisited season after season, so a randomly held-out station usually has a near neighbour in training, often the same station in another year. A model can then score well by recognising where it has been rather than what makes a patch, and the ROC curve cannot tell those apart.On the mock config it comes out at 0.2, saying that run's 0.85 AUC is optimistic — a real finding the evaluation could not previously produce. It goes into
evals.csvas anssbrow rather than a file of its own, because it is the caveat on theroc_aucthree rows above it and a reader who has to go looking will not.This needed the fitted model to know where its stations were.
model_dataholds predictors only and coordinates must not become predictors, so they are carried beside it, indexed by the same.rowthe held-out predictions use.projection_overlap()— Schoener's D and Warren's I. Answers whatcompare_runs()cannot: not which surface scores better, but whether the two are the same surface. Two runs can rank stations equally well and disagree completely about where the habitat is.Both statistics run high, and that is documented rather than left to be misread: two surfaces of independent noise score about 0.7 and 0.9, because both spread probability over the same cells in similar proportions. That is the floor for surfaces of this shape, not near-identity.
Derived covariates: two things that were unaskable
Not fancyfx work, but found while testing the app.
A derived covariate is now offered whether or not its source is modelled. Integrated chlorophyll without chlorophyll itself is an ordinary thing to want — the accumulated bloom is what feeds the animals, the instantaneous value is a different quantity — and it could not be asked for:
derivoce_choices()looped over the selected covariates, soCHL_intappeared only once CHL was a predictor, and CHL could not then be removed. The loop now covers the whole catalogue, and options costing a download say so in their group.Current speed can now be differentiated. UO and VO could each be, and the speed could be computed, but the speed could not then be differentiated — and the gradient of the speed is what a front in the flow is: two components can each change steeply while the speed is constant, which is a turn rather than a shear. It is also the original pipeline's
uv_grad. This needed a step that depends on another step rather than on a download; ordering needs no sorting, since the choices already list a dependency before anything reading it.And a bug that came with it: picking
speed_gradcomputesspeedon the way, which would have arrived as a predictor nobody asked for — exactly the ingredient-vs-predictor problem the download side already solved.derivoce_dependency_columns()names those and the app excludes them.Not taken: the four evaluation plots
Investigated rather than assumed.
evaluation_pairs()re-predicts, and taupatch's ROC/threshold/calibration come from pooled out-of-fold predictions — each station scored by the fold model that never saw it. fancyfx says so itself, warning and captioning in-sample figures; on arecipesworkflow it does not even get that far, since the engine's response is the baked..yand the call errors.held_out()in fancyfx#1 unblocks this, and it is the natural next step — left out of this PR to keep it reviewable.comparePlots()for ensemble members is also blocked: fancyfx asks marginaleffects forscale="response", but a classification workflow needstype="prob". marginaleffects supportsworkflownatively, so it is a gap in fancyfx's scale resolution — a third upstream generalisation, smaller than the two already done.Verification
_R_CHECK_LIMIT_CORES_=TRUE.patchlabels across a 4-fold cycle left every fold single-class. Fixed by shuffling, and noted because it looked like a code bug for a moment.🤖 Generated with Claude Code