Repository navigation
revamp functions that run executable and associated cleanup - #722
Conversation
- thanks to @yukio-takeuchi - see #460 (comment)
- 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
|
@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. |
k-doering-NOAA
left a comment
There was a problem hiding this comment.
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.
|
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! |
|
@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. |
|
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. |
|
@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. |
A bunch of major changes included here as discussed in #460:
check_model()is nowcheck_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:run_SS_models()is nowrun()and has easier defaults and bettersupport for having the Stock Synthesis executable in your path (by relying on
check_exe())dirinstead ofdir,mydir,File, ormasterdir, and now defaulting to current working directory)show_in_console = FALSEwill pipe output to a text file keeping R console cleaner while models runSSbootstrap()Here are a couple design choices that I would appreciate additional input on before removing "draft" from this Pull Request:
The functions that run the executable use
...to pass additional arguments torun(), such asextras,show_in_console, andskipfinishedrather 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: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).