nmfs-ost / ss3-source-code

The source code for Stock Synthesis (SS3).
https://nmfs-ost.github.io/ss3-website/
Creative Commons Zero v1.0 Universal
37 stars 17 forks source link

allow custom output filenames #545

Closed Rick-Methot-NOAA closed 9 months ago

Rick-Methot-NOAA commented 10 months ago

Resolves issue #319

What tests have been done?

ss3.exe -nohess -stopph 3 modelname ss4you

produces expects to read ss4you.par and will write ss4you.par

<-- - [x] No test files are required for this pull request. -->

What tests/review still need to be done?

test with mcmc

Is there an input change for users to Stock Synthesis?

No required change. Provides option to change filename prefix at command line. This needs to be documented in command line section of the manual

A new command line argument will set the filename for all ADMB output files to "base_modelname.[ext]" where base_modelname can be read from command line with command: modelname followed by the desired name e.g. ss3_win.exe modelname ss4you When readparfile is used, SS3 will attempt to read ss4you.par. ss4you will be used as prefix for output files such as ss4you.par and ss4you.cor if requested modelname.par is not found, then will attempt to read from ss3.par, then ss.par, then will fail with warning. Whatever name is read, the write will be to modelname.par. Modelname defaults to ss3.par unless changed by the modelname command. Which has default of ss3.par. So, if modelname option is not used and the model reads from ss.par, it will write to ss3.par. Use of modelname ss will allow writing to ss.par for backward compatibility.

e-perl-NOAA commented 10 months ago

@Rick-Methot-NOAA do you now HAVE to give it modelname each time you run the model or is the default ss3?

Rick-Methot-NOAA commented 10 months ago

default is ss3. Which means it will not read an existing ss.par.

Rick-Methot-NOAA commented 10 months ago

Ahh. I see GHA failing because trying to read the new default ss3.par but the file is named ss.par. I am inclined to leave this one on the user rather than add a check for existence of ss3.par. OK?

e-perl-NOAA commented 10 months ago

Had to make changes to the ss3-test-models repo to get GHAs passing. Please see PR #56 in the test-models repo. I will need to make a change in the workflows to go back to using the main branch after that PR is approved and merged. I used the branch I made changes to just to make sure the actions would run here.

e-perl-NOAA commented 10 months ago

So does modelname ss4you also tell it to read ss4you.par rather than ss3.par in addition to creating ss4you.par? Just wanting to make sure I have it all ironed out to describe in the documentation. Also, would we want all references to ss.par in the user manual change to ss3.par?

Rick-Methot-NOAA commented 10 months ago

Relying to Elizabeth: yes. Whatever the value is for modelname would be used for both reading and writing the .par file.

e-perl-NOAA commented 9 months ago

Okay, then with this change should we change the user manual and other documents to say 'ss3.par' instead of 'ss.par'? I also think this will be a huge change for some people. I will create a draft PR of my changes for you to initially look at.

Rick-Methot-NOAA commented 9 months ago

good point. we could keep default at ss.par and anyone could change to ss3.par using the modelname command line argument

e-perl-NOAA commented 9 months ago

I guess the difficulty is if the modelname specification is also needed to read the old ss.par file instead of the new default ss3.par file.

iantaylor-NOAA commented 9 months ago

I think the challenge here is that ss3.par will be simpler and more consistent going forward, it just requires a change for some folks who might have written some scripts or other tools based on the old name. However, if you try to use the par file and it doesn't have the right name, SS3 exits with the message "Error trying to open parameter input file ss.par" (in the future it would say "ss3.par"). I think that error (combined with a comment in the release notes) should sufficient to point folks in the right direction.

Rick-Methot-NOAA commented 9 months ago

Let me look into an approach like: if_exist ss.par, then read it, else look for modelname.par

e-perl-NOAA commented 9 months ago

@iantaylor-NOAA I may need your help looking into why the r4ss test is failing. Looking into it this morning. Will let you know if I figure it out before my afternoon meetings.

e-perl-NOAA commented 9 months ago

@iantaylor-NOAA This error is occurring locally too, it seem to be an error in the SS_plots function and the get_DM_sample_size sub-function within that. The error I am getting locally which is the same one that I see in the github action run is Error in if (CompError == 1) { : missing value where TRUE/FALSE needed

iantaylor-NOAA commented 9 months ago

It looks like test-r4ss-with-ss3 is now passing thanks to the changes in https://github.com/r4ss/r4ss/pull/899.

e-perl-NOAA commented 9 months ago

Thanks @iantaylor-NOAA!