Skip to content

Bug fixes - #1

Open
seananderson wants to merge 7 commits into
DeepWaterIMR:mainfrom
seananderson:SA-fixes
Open

seananderson wants to merge 7 commits into
DeepWaterIMR:mainfrom
seananderson:SA-fixes

Conversation

@seananderson

Copy link
Copy Markdown

Thanks for working on this package! This PR was Claude assisted. Summary below:

This branch fixes several places where the comparison and retrospective functions silently changed the model they were meant to reproduce. It also makes the helpers independent of the X/Y column names and of an attached ggOceanMaps, and turns on tests and the vignette in R CMD check. It is merged up to current main (0.1.3), so it includes the model-selection functions.

Main fixes

  • Refits keep the original model settings. sdmTMB_retro(), compare_meshes(), sdmTMB_mesh_cv() and compare_surveys() (with compiled_model) now refit from the original sdmTMB() call, as update.sdmTMB() does. Before, settings such as spatial_varying, time_varying, extra_time, weights, priors, share_range, anisotropy and reml were dropped. sdmTMB_mesh_cv() also dropped the offset. The call's arguments are evaluated on the main process so they reach parallel workers.
  • Coordinate and time columns are no longer hard-coded. Fitted models use object$spde$xy_cols. compare_surveys() gains xy_cols, and the comparison functions gain coord_multiplier. Time columns not named year now work.
  • Index and plot fixes.
    • The reference model is always drawn in black.
    • Index plots no longer give NA colours with more than seven series.
    • Peel order is kept with more than ten peels.
    • predict_args now reaches get_index_split().
    • plot_variance_partitioning() only uses link-scale SDs.
    • The "Terminal-year rel. diff." label replaces the misleading Mohn's rho label.
  • Other robustness fixes.
    • Non-integer mesh cutoffs work.
    • A failed reference refit warns instead of erroring.
    • A numeric area vector can be subset per row.
    • Barrier meshes are kept, or you get a warning when they can't be.
    • Failed fits report the real error.
    • Fits that fail sanity() are flagged.
    • Map helpers work when ggOceanMaps is loaded but not attached.
  • Bug in the merged model-selection code. sdmTMB_selection() passed the list returned by .setup_parallel() to future::plan(), which corrupted the plan for later parallel calls. It now uses .restore_parallel().

Breaking changes

  • sdmTMB_retro(refit_meshes = TRUE) now requires cutoff.
  • sdmTMB_retro()'s n_workers defaults to 1 (was nyears). With n_workers = 1 the user's current future::plan() is used.
  • The %>% re-export is removed.
  • ggplot2 >= 3.5.0 is required.

Dependencies and data

  • ggOceanMaps moves from Depends to Suggests, and spatial plots fall back to a plain map without it.
  • plyr, magrittr and viridis are dropped.
  • A small sebastes_bathy dataset and sebastes_depth() let examples, the vignette and tests avoid the ~85 MB bathymetry download.

CI and tests

  • tests/ and vignettes/ are no longer build-ignored, so CI runs the tests and builds the vignette.
  • New test-refit.R checks that refits reproduce the original fit and index.
  • devtools::check() gives 0 errors, 0 warnings and 0 notes locally.

seananderson and others added 7 commits October 4, 2026 18:04
…n CI

Refits in sdmTMB_retro(), compare_meshes(), sdmTMB_mesh_cv() and
compare_surveys() rebuilt the sdmTMB() call from a few fields, silently
dropping spatial_varying, time_varying, extra_time, weights, priors,
share_range, anisotropy, reml, etc. (and the offset in sdmTMB_mesh_cv()).
They now refit from object$call via new internal .refit_args()/.refit()
helpers, evaluating arguments on the main process so they reach future
workers and subsetting numeric offset/weights to the retained rows.

Also:
  - Use object$spde$xy_cols instead of hard-coded X/Y; add xy_cols to
    compare_surveys().
  - Stop build-ignoring tests/ and vignettes/ and set NOT_CRAN on CI so
    R CMD check actually runs them; add refit-reproduces-original-fit tests.
  - Move ggOceanMaps to Suggests with a plain-map fallback; drop plyr,
    magrittr (and the %>% re-export) and viridis.
  - sdmTMB_retro(): require `cutoff` when refit_meshes = TRUE; default
    n_workers = 1. Respect the user's future::plan() when n_workers = 1 and
    restore future.globals.maxSize on exit.
  - Report actual fit errors and flag fits failing sanity().
  - Untrack memory/, CLAUDE.md, AGENTS.md and docs/ (pkgdown deploys to
    gh-pages); start a development NEWS section.
- Keep the sanity table's shape when a survey model fails to fit
- Always draw the reference model in black; no NA colours for many series
- Keep non-integer mesh cutoffs; handle a failed reference refit
- Subset a numeric `area` vector with the prediction grid
- Support time columns not named `year` (index output always uses `year`)
- Add `coord_multiplier` to the comparison functions and spatial maps
- sdmTMB_mesh_cv(): default xy_cols from the mesh; catch an NA CRS
- Keep barrier meshes in retro peels; warn when new meshes drop them
- plot_variance_partitioning(): only use link-scale SDs
- Relabel the terminal-year difference in model/mesh comparison plots
- plot_prediction_histogram(): put offset models on the density scale
- Pass sdmTMB_retro(predict_args) to get_index_split(); keep peel order
- Use `transform` instead of the deprecated ggplot2 `trans` argument

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Combine upstream model-selection functions and plot_mesh options with the
refit/xy_cols fixes. Keep AGENTS.md, CLAUDE.md and memory/ deleted.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
All three are already listed in .Rbuildignore.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
- sdmTMB_selection(): restore the future plan with .restore_parallel(); passing
  the .setup_parallel() list to future::plan() corrupted the plan and broke
  later parallel calls.
- test-refit: compare against get_index_split(bias_correct = FALSE), matching
  the function defaults.
- NEWS: number the development section so R CMD check can parse it.

Co-Authored-By: Claude Sonnet 5.5 <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

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant