Skip to content

[feat]: More fims plots runs test, follows #469 - #473

Open
ericward-noaa wants to merge 7 commits into
nmfs-ost:mainfrom
ericward-noaa:more-fims-plots-runs-test
Open

ericward-noaa wants to merge 7 commits into
nmfs-ost:mainfrom
ericward-noaa:more-fims-plots-runs-test

Conversation

@ericward-noaa

@ericward-noaa ericward-noaa commented Nov 7, 2026 •

Copy link
Copy Markdown
Contributor

What is the feature?

stockplotr can show the fit to an index (plot_index()), but it has no standard diagnostic for whether the residuals of that fit are random. The residual runs test is a standard diagnostic in ss3diags (SSplotRunstest()) and JABBA, and it’s useful for FIMS models, which can include several survey and fishery indices. This PR adds plot_runs_test(), which works on any convert_output() output (SS3, BAM, FIMS) and on the package’s example data.

Follows PR #469

How have you implemented the solution?

plot_runs_test():
It plots log residuals, log(observed) − log(predicted), over time for each index.
A shaded band shows the three-sigma control limits, with sigma from the average moving range.
The band is green when a one-sided runs test passes (p ≥ 0.05) and red when it fails. Residuals outside the limits are filled red.
It has fleet, module, ncol, interactive, make_rda and figures_dir arguments, consistent with the other plot functions.

Does the PR impact any other area of the project, maybe another repo?

save_all_plots():
It runs every exported plot_* function, so it now also saves runs_test_figure.rda.
test-save_all_plots.R adds that file to the expected list.
Captions: captions_alt_text_template.csv gains one row (runs_test). Existing rows are unchanged.

PR contribution checklist

Thank you for contributing to stockplotr! Please check the boxes below to confirm this pull request (PR) is ready for review. Edit this PR description to add the release indicator and any explanations.

  • I have read and adhered to the contributor guide.
  • I have kept this PR focused on one plot where applicable. For multiple features, I have used separate PRs (stacked PRs are welcome if needed).
  • I have aimed to keep this PR to no more than 750 lines, excluding tests and example data (for example, up to 750 lines for a new function).
  • If I have provided an example dataset, I have explained why the existing sample datasets are not sufficient.
  • I have tested that my changes are compatible with all example outputs in the "data" folder (example_data.rda; pollock.rda; ss3_simple.rda; vermilion_snapper.rda).
  • If I didn't follow the development workflow, I have explained why in this PR.
  • If applicable, I've added the plot to save_all_plots() and updated the associated tests in test-save_all_plots.R.
  • After opening this PR, I have checked that my own tests and spellcheck pass (spelling::spell_check_package()). If other checks fail, I have flagged them for the package maintainers to help diagnose and remedy.
  • I have requested review from one package maintainer (Sam or Sophie).

ericward-noaa and others added 6 commits November 6, 2026 07:12
convert_output() passed FIMS output through unchanged, so `estimate` and
`year` were empty and no stockplotr plot could use it. The new
conout_fims() reshapes `FIMS::get_estimates()` output (or a FIMSFit object,
or an .rds file with either):

- Each data point (index, landings, age and length compositions) is split
  into "*_observed" and "*_predicted" rows, with the observation error and
  negative log-likelihood on the observed row, as for SS
- `estimated` becomes `estimate`, and the new `start_year` argument converts
  FIMS year indices to years; `era` is "time", or "fore" after the last year
  of data.
- fims_var_names.csv maps current FIMS labels (e.g., expected_recruitment,
  numbers_at_age, log_devs) to the standard labels.

Also fixes:
- convert_output() ignored a user-supplied `model` when `file` is an object.
- plot_index() matched every label containing "index" (e.g., index_log_q in
  Rceattle output or index numbers at age in FIMS output); it now plots only
  index_observed and index_predicted.
- plot_index() failed for a single index fleet, because the fleet facet was
  added back after process_data() dropped the column.
these tests were failing, should pass now
Residuals of the fit to each index over time, with three-sigma control
limits and a runs test for non-random residuals (as in ss3diags/JABBA).
Works on convert_output() output for SS3, BAM, and FIMS.
@ericward-noaa ericward-noaa changed the title More fims plots runs test [feat]: More fims plots runs test Nov 7, 2026
@ericward-noaa ericward-noaa changed the title [feat]: More fims plots runs test [feat]: More fims plots runs test, follows #469 Nov 7, 2026
@ericward-noaa

Copy link
Copy Markdown
Contributor Author

@Schiano-NOAA @sbreitbart-NOAA ready for review

Comment thread R/conout_fims.R

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

was devtools::document run when this was made? I don't see new documentation.

Comment thread R/convert_output.R
#' in the file path provided.
#'
#' Default: NULL
#' @param start_year Number. First year of the model, for FIMS output only.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Is it possible so FIMS outputs years too or instead of this?

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.

The background here is that FIMS::get_estimates returns year_i, starting at 1. The FIMSFit object doesn't keep the data's FIMSFrame (where the start year lives -- can be extracted with FIMS::get_start_year() )

I think there's a few options,
(1) Keep start_year as is
(2) Ask FIMS to add years to output
(3) let the start_year argument also be a FIMSFrame, and then the function could call FIMS::get_start_year()

Others?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm chatting with Kelli on thursday to align naming and output from stockplotr and FIMS. Ideally it should be a 1:1 so the conout_fims would be moot at some point. I will bring up the years in FIMS to her then

Comment thread R/plot_runs_test.R Resolved
@ericward-noaa

Copy link
Copy Markdown
Contributor Author

plot_runs_test(stockplotr::example_data, interactive = FALSE)

runs_1

@Schiano-NOAA Schiano-NOAA left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think this is almost good to go. It works for all of the example data and is a good way to incorporate the FIMS output into the workflow.

Could you please add the key quantities that are generated into the key_quantity_template.csv found in the inst/resources folder. In other plots that are exported to an rda, the key quantities get filled in to the sheet to provide a record of these quantities for future referece. To see what happens run the following example:

plot_biomass(stockplotr::example_data, module = "TIME_SERIES", make_rda = TRUE)

To understand how it works see here:

fill_in_kqs <- function(df, ...) {

and
export_kqs <- function(..., dir=getwd()) {

Once that is done, ping me and I will merge 😄

@ericward-noaa

Copy link
Copy Markdown
Contributor Author

Thanks @Schiano-NOAA , I missed the key quantities stuff before. Fixed now. To clarify, I think these should be merged in first:

#465
#468
#469

@Schiano-NOAA

Copy link
Copy Markdown
Collaborator

@ericward-noaa thanks! I appreciate the update and the suggested merge order. I am working on the others now

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.

2 participants