[JZahner1]

New plotting and table functions will automatically be generated with their default parameters by save_all_plots, without requiring manual changes to the save_all_plots function. This is accomplished by pulling a list of all available plot/table functions based on the existence of a plot_.R or table_.R file in the "R/" directory.

This does require that 1) the name of a plotting function matches with the name of an R file (e.g., plot_spawning_biomass() -> plot_spawning_biomass.R`, and 2) there can only be one function per R file.

This current implementation is sub ideal, but can potentially be improved by pulling available plot/table function directly from the stockplotr NAMESPACE (though we would need to be careful to exempt utility plotting functions (e.g., plot_timeseries).

Users retain the ability to customize individual plots within the save_all_plots function by specifying a named list of parameters as an input parameter. For example, to customize the behavior of plot_spawning_biomass:

save_all_plots(
    example_data,
    plot_spawning_biomass_options = list(
        ... # Whatever custom options you want to pass directory to the underlying function
    )
)

The save_all_plots test does need to be updated with the name of the output file for each new function that is added. This could possible be automated in the future as well.

[JZahner1]

@sbreitbart-NOAA the addition of the "extract_sis_data" tests results in a race condition with the "save_all_plots" test, due to coincident file writing and deletion to the same directory. How would you like to handle this? One way is to merge the two sets of tests, since testthat/parallel runs test files in parallel rather than functions. This feels like a bad way to fix this issue. Another way would be to this would be have each test write figures and temp file to a custom directory (e.g., test-save_all_plots.R writes to tests/testthat/test-save_all_plots/figures). This is more extensible, but requires adding functionality to export_kqs that allows for writing those files to custom directories rather than getwd(). Thoughts?

I have a tested/verified version of option 2 here: https://github.com/JZahner1/stockplotr/tree/test-race-conditions

[sbreitbart-NOAA]

@JZahner1 A few thoughts:

1. I really like your idea for changing save_all_plots(). The result is much easier to run (and update with new functions), and I think the downside (less customization) is worthwhile because, as you mention, users can run the individual plots with customized arguments if they want. It's living up to its purpose: to get all of the plots exported in one go, relatively quickly. I think the Roxygen documentation should be updated to explain that clearly.
2. The three plots you identified as needing their own module-specifying arguments are specific for the SS3 example_data; other models' data will require other plots' modules to be specified. The future feature that automatically adds new functions to this function (e.g., GH action) could easily add a new module-specifying argument for each plotting function. At that point, it would act as a helpful reminder/opportunity to identify which module should be added as the default, if we're assuming the default/most commonly-used model to be SS3 (see next point).
3. I don't think that this PR should close the linked issue Brainstorm ways to automatically update save_all_plots() #432 as it doesn't fully automate the addition of plots to save_all_plots() in the case that a new argument needs to be made. A feature that did accomplish that (e.g., a GH action) would accurately address the issue.
4. Re: the race condition issue: I don't mind putting the tests into the same file. I appreciate the work you did to allow the tests to run in parallel, but I'd prefer to keep it simple and leave export_kqs() unchanged if it's only a small matter of testing. If this is a more important choice than I realize, I'm happy to explore other options.

Thanks for putting in the work to reimagine this function!

[JZahner1]

1. I guess it depends exactly how you expect this function to be used. If the desire is for it to simply output a ton of default plots with limited user input, then Id agree that the roxygen should be updated to reflect that intent and push users towards running individual plotting functions if they want additional customizations. As the function is currently written, it supports users applying customizations directly, albeit its a bit funny, and somewhat difficult to explain as part of the roxygen.
2. Thats a good point. Because example_data is built on an SS3 model the tests (and default function pars) tend to neglect other model types.
3. That fine with me. Im not knowledgeable about GH actions, so I will leave that to you all to figure out.
4. Im going to push back on this for a few reasons:
   a) test files really should be for a single function at a time, otherwise identifying where failing tests are located becomes a nightmare;
   b) combining the test files will, I believe, rather substantially increase the time required by the tests suite, bc these two tests are consistently the longest running. Keeping them separate files makes running them in parallel much more reasonable and efficient;
   c) I think knowingly leaving a potential race condition in the codebase is not great software practice and could result in additional, as yet unforseen, bugs in the future; rooting it out now might save some time/sanity later when the package is more complex;
   d) I think users should have (and likely would want) the ability to customize where the kq files get written too. I can imagine scenarios where a user would have multiple versions of an assessment model and want to write plot/table/captions output to different directories as a means of saving different versions of plots/tables. Given that the implementation for this is not very difficult, I think it makes sense to implement this now, rather than waiting until there are dozens more plots that have to be changed.

[iantaylor-NOAA]

Sorry, I'm late to this conversation and maybe I should comment on issue #432 instead, but this PR seems to be where the discussion is most active.

Here are a couple quick thoughts:

1. As a user, I want a single function call that will create all the figures and tables for a single model and I appreciate @JZahner1 helping stockplotr move in that direction by streamlining that function. This also means figuring out how to avoid things like the required module-specifying arguments for SS3 models (plot_abundance_at_age() can always use the numbers-at-age natage module).
2. As a developer, I see that there's value in having more control over which functions get called and in which order, especially as stockplotr expands to include functions for features not present in all models (e.g. growth as in [feat]: plot_age_to_length #403), or that depend on multiple models (e.g. sensitivity tables in [feat]: Sensitivity runs table #346).

Compared to the task of creating an entirely new plotting function, the chore of adding that function to save_all_plots() doesn't seem that onerous, so an alternative would be a github action that fails if your new function doesn't get called by save_all_plots() (and you haven't specified somewhere that it should not be included in the set).

[JZahner1]

@iantaylor-NOAA yah I agree on both perspectives. Both SS3 (via r4ss) and WHAM have singular functions that generate default versions of everything, so I would guess that most users would expect that feature to exist in any new plotting software.

The way I designed the function parameters was so that as few plot-specific function parameters are required as possible. The three options lists that currently exist as required parameters because of specific module names are more of an issue with the default behaviors of those specific functions (e.g., the first MODULE_NAME entry in the data isn't the one we actually want to use). Because the default behavior of filter_data uses the module we want to plot with, none of the other plotting functions require a default parameter with a module name. Though, users can optionally add such a parameter (using plot_*_options=list(...) syntax) or other plot-specific values.

I definitely agree that it's not ideal to keep the few required parameters for specific functions that have strange default module behavior, but I think that's a whole separate issue to deal with.

Regarding 2: I do think it would be valuable to be do something like r4ss where each plot gets a number and users can customize which plots get output by specifying a vector of numbers corresponding to each plot.

[Schiano-NOAA]

Does this also include other arguments like units? Could you provide an expanded default like list("module = "TIME_SERIES", unit_label = NULL, ...) I think this would help make things a bit more clear (at least for me so i'm thinking for the users as well)

[Schiano-NOAA]

I'm not sure how this would work for a large number of functions and not get confusing so I wonder if we take it out for this one. The users can run the individual functions they want to if they want this functionality

[JZahner1]

So, this WOULD probably get confusing if you are customizing a large number of functions. I dont think that's really the intended use of this function, but I dont see the harm in allowing the option.

[Schiano-NOAA]

Would the option not apply to every other function that also uses the ... to inherit?

[JZahner1]

So it would like this:

save_all_plots(
    dat = example_data,
    figures_tables_dir = fs::path("outputs/figures"),
    plot_recruitment_options = list(
      module = "TIME_SERIES",
      unit_label = "Millions of Recruits" # customize unit label for recruitment plot
    ),
    plot_spawning_biomass_options = list(
      ref_line = "unfished"     # customize reference line for spawning biomass plot
    ),
    plot_abundance_at_age_options = list(
      scale_amount = 1,         # custom scale for abundance-at-age plot
      proportional = FALSE      # don't proportationalize abundaance-at-age 
    ),
    plot_spr_options = list(
      ylab = "Fishing Intensity (1-SPR)/(1-SPR_ref)" # custom Y label for SPR plot
    ),
    plot_fishing_mortality_options = list(
      facet = "fleet",  # facet fishing mortality plot by "fleet"
      group = "fleet"   # group fishing mortality plot by "fleet"
    )
)

where each plot/table function could have its own named list

[JZahner1]

There's probably a better way to explain it in the documentation. I added the above as an example to roxygen.

[Schiano-NOAA]

Okay perfect that clarifies it very well imo

[Schiano-NOAA]

Reminder to make this change before merge 😄

[JZahner1]

Just remove the comment or adjust the implementation? Changing the implementation requires some additional steps to make me sure we only get true plotting functions and not helpers like plot_timeseries.

[Schiano-NOAA]

I would say the latter so it only pulls the intended plots and figures and not helpers. We will probably be using this function to help us make the dashboard

[JZahner1]

So just drop the TODO then? And we can worry about the implementation another time?

[Schiano-NOAA]

No I think we should do this now

[JZahner1]

Okay. For now, what I will do I make a list of plot/table functions that we can exclude after pulling all of the functions from the package NAMESPACE. Depending how many helper functions get created and added in the future, this may eventually become untenable, but it will work fine for now.

[Schiano-NOAA]

I think you can delete this

[Schiano-NOAA]

I like the idea of this, but I think it leaves some confusing to the users. Could we change it to something indicating this is exporting, then add a warning message later such as "Failed to execute: {func_name}"?

[Schiano-NOAA]

Suggested change

	#' save_all_plots(dat,
	#' save_all_plots(
	dat = example_data,

[Schiano-NOAA]

Sorry for the delayed review. I absolutely LOVE this improvement to this function. It worked as intended for me when I ran my own test and used the example. Please make the small adjustments in my comments then the only thing left to do is fix the failing tests then it's good to merge. I might be able to assign copilot to do that if you're okay with it.

Tysm!!

[Schiano-NOAA]

See above comments -- meant to comment and not approve so we know not to merge until small changes are made

[sbreitbart-NOAA]

1. I guess it depends exactly how you expect this function to be used. If the desire is for it to simply output a ton of default plots with limited user input, then Id agree that the roxygen should be updated to reflect that intent and push users towards running individual plotting functions if they want additional customizations. As the function is currently written, it supports users applying customizations directly, albeit its a bit funny, and somewhat difficult to explain as part of the roxygen.

   2. Thats a good point. Because example_data is built on an SS3 model the tests (and default function pars) tend to neglect other model types.

   3. That fine with me. Im not knowledgeable about GH actions, so I will leave that to you all to figure out.

   4. Im going to push back on this for a few reasons:
      a) test files really should be for a single function at a time, otherwise identifying where failing tests are located becomes a nightmare;
      b) combining the test files will, I believe, rather substantially increase the time required by the tests suite, bc these two tests are consistently the longest running. Keeping them separate files makes running them in parallel much more reasonable and efficient;
      c) I think knowingly leaving a potential race condition in the codebase is not great software practice and could result in additional, as yet unforseen, bugs in the future; rooting it out now might save some time/sanity later when the package is more complex;
      d) I think users should have (and likely would want) the ability to customize where the kq files get written too. I can imagine scenarios where a user would have multiple versions of an assessment model and want to write plot/table/captions output to different directories as a means of saving different versions of plots/tables. Given that the implementation for this is not very difficult, I think it makes sense to implement this now, rather than waiting until there are dozens more plots that have to be changed.

1. I think it'd be sufficient to add a sentence or two like this, to the details section: "This primary purpose of this function is to quickly export all available figures and tables. We recommend running individual plotting functions for greater customization of figures and tables."
2. Please update the function to allow for specification of module-specifying arguments (and the "default" indicators in the Roxygen documentation)
3. Please unlink Brainstorm ways to automatically update save_all_plots() #432 from the "Development" tab on the right so this PR doesn't close that issue
4. You've convinced me! Please go ahead with this test implementation

[JZahner1]

@sbreitbart-NOAA

re 2: Is this not already done? If not, could you be more specific about what you are looking for?
re 4: okay, sounds good. Would you prefer I simply merge that race condition fix into this PR directly on my end, or do you and @Schiano-NOAA want to review it separately?

[Schiano-NOAA]

@sbreitbart-NOAA

re 2: Is this not already done? If not, could you be more specific about what you are looking for? re 4: okay, sounds good. Would you prefer I simply merge that race condition fix into this PR directly on my end, or do you and @Schiano-NOAA want to review it separately?

I'm good with a separate PR so it's more manageable

[sbreitbart-NOAA]

@sbreitbart-NOAA

re 2: Is this not already done? If not, could you be more specific about what you are looking for? re 4: okay, sounds good. Would you prefer I simply merge that race condition fix into this PR directly on my end, or do you and @Schiano-NOAA want to review it separately?

-2. I see what you did, by adding an argument that allows all plotting functions' modules to be chosen. I haven't seen a semi-anonymous function like that implemented before. Interesting!
-4. Please make it into a separate PR that can be merged (or stacked) into this one

[JZahner1]

okay. let me see what's easiest, bc I made the race-conditions branch off of this PR. Not sure how linked they are.