Skip to content

revamp functions that run executable and associated cleanup - #722

Merged
iantaylor-NOAA merged 49 commits into
mainfrom
system2_issue456
Jul 26, 2022
Merged

iantaylor-NOAA merged 49 commits into
mainfrom
system2_issue456

Conversation

@iantaylor-NOAA

@iantaylor-NOAA iantaylor-NOAA commented Jul 21, 2022 •

Copy link
Copy Markdown
Contributor

A bunch of major changes included here as discussed in #460:

  • Major revamp of all functions that run the Stock Synthesis executable
  • check_model() is now check_exe() and supports having an executable in the path (support workflow of having SS executable in the path rather than directory with model files #429). Here's the new "description" section in the function documentation:

    First searches path for the specified executable name and returns the location of that file if found. If not found in path it then searches the specified directory. Linux systems have an existing executable utility /usr/sbin/ss in the path. If exe = "ss" and this file is found by this function, it will be ignored based on the smaller file size relative to the SS3 executable. Linux users who want to use the workflow of having an SS3 executable in their path should name the file something else, such as ss3 or ss_linux.

  • run_SS_models() is now run() and has easier defaults and better
    support for having the Stock Synthesis executable in your path (by relying on check_exe())
  • Functions have more consistent inputs (e.g. always dir instead of dir, mydir, File, or masterdir, and now defaulting to current working directory)
  • Input show_in_console = FALSE will pipe output to a text file keeping R console cleaner while models run
  • Adds dependency on 'tidyr'
  • Removes never-completed function SSbootstrap()

Here are a couple design choices that I would appreciate additional input on before removing "draft" from this Pull Request:

  1. The functions that run the executable use ... to pass additional arguments to run(), such as extras, show_in_console, and skipfinished rather than add a bunch of new inputs to each of those functions (previously not all these options were available or they were controlled by inconsistently named inputs). Benefit is that the function isn't cluttered with as many inputs, but the drawbacks are that these options are less visible and error message associated with incorrect inputs are further removed from the function the user called as in the message where the user calls the jitter function and gets an error from the run() function:

    > SS_RunJitter(dir = "inst/extdata/simple_small", Njitter = 1, nonsense = "blah")
    Error in run(dir = dir, exe = exe, verbose = verbose, ...) : 
      unused argument (nonsense = "blah")
    
  2. Can we rename the functions that call the executable as follows to avoid a bunch of inconsistent capitalization? I didn't do this in the pull request because it makes reviewing changes harder (I probably should have held off on renaming run_SS_models() to run() for that reason as well).

old new
SS_RunJitter() jitter()
SS_doRetro() retro()
SS_profile() profile()
SS_tune_comps() tune()

- update to simple_small when smaller_test_model_206 branch is merged
- also devtools::document() SS_profile()
- function was formerly check_model()
- resolves #429 (allow exe in path)
- no longer a warning if exe is path
- lots of cleanup, including aligning inputs and deprecating old ones
- bump version number to 1.46.0
also change "masterctlfile" to "oldctlfile" in SS_profile()
- adds dependency on `tidyr`
- starts update of NEWS.md
- #460
@iantaylor-NOAA
iantaylor-NOAA marked this pull request as ready for review July 25, 2022 21:09
@iantaylor-NOAA

Copy link
Copy Markdown
Contributor Author

@k-doering-NOAA, I think this PR is now ready. I'm not sure how well GitHub will identify the additional changes made since your review of the draft version of this Pull Request, but the focus has been on the issues that you and @kellijohnson-NOAA identified, as well as issue #429 and finally spell check.

Comment thread R/check_exe.R Outdated
Comment thread R/check_exe.R Outdated
Comment thread R/getADMBHessian.R
Comment thread R/getADMBHessian.R Outdated
Comment thread R/run.R Outdated
Comment thread R/run.R Outdated
Comment thread inst/WORDLIST
Comment thread man-roxygen/exe.R

@k-doering-NOAA k-doering-NOAA left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Great work, @iantaylor-NOAA ! I support merging this into main, with a few minor wording changes in the documentation. The biggest concern I had was over the how to construct values for deprecated inputs in getADMBHessian, but it should be quick to decide on what to do.

@k-doering-NOAA

Copy link
Copy Markdown
Contributor

Also, there is an option to look only at changes since you last reviewed something on Github, which is a nice feature for big PRs like this one!

@iantaylor-NOAA

Copy link
Copy Markdown
Contributor Author

@k-doering-NOAA, thanks for the really valuable review! I'll wait a few hours until the tests are completed and give you a chance to look at the changes I made if you want, and then merge if no further issues are identified.

@k-doering-NOAA

Copy link
Copy Markdown
Contributor

All looks, good, thanks @iantaylor-NOAA, I have no other changes to suggest.

Just a note about the failing codecov check - it's failing because the coverage decreased by more than 2% (set in the codecov.yml file. I think this is probably due to taking out some tests for 3.24 code, so I don't think it needs to be worried about.

@iantaylor-NOAA

Copy link
Copy Markdown
Contributor Author

@k-doering-NOAA, thanks for the info on the codecov check. Those functions were deprecated in a previous pull request #718, but the coverage didn't change, perhaps because the version wasn't yet incremented above the one noted in the calls to lifecycle https://app.codecov.io/gh/r4ss/r4ss/pull/718. Looking through the files, it indeed seems to be the read/write 3.24 files that had the big drop in coverage. When we delete those files in some future version it will go back up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants