reduce output to console for ADMB 13#312
Conversation
|
For reference, this is what the admb console output looks like for the simple admb model: This is what the SS3 console output looks like (including current changes made to this branch): |
This info is available with the default console for ADMB 13.
|
I just looked through all the changes. This all seems great to me. Moving more into echoinput.sso ensures that nothing is lost and the console output is easier to read with less info. |
That was what I was thinking, too, more details in echoinput rather than the console. And you are right about the run detail setting, thanks @iantaylor-NOAA! I'll try it with 1 and 2 as well. Any idea what most people would use as a default setting? |
|
I really can't speak for what others use for rundetail setting in their starter file with SS3, but here's what I do:
However, with the revised ADMB output it's possible that I'll be inspired to use 0 more often. |
|
My logic similar to Ian's. |
Send more messages to echoinput than cout
|
Johnoel got me set up with ADMB 13.0, so I can test now also. He is open to tweaks on the ADMB side if you see something in the console output that could be even better. make_ss_safe.bat has a few more explicit references now. |
|
Here is what option 0 output looks like now (using ADMB 13): I'm fairly satisfied with how this looks, but please feel free to suggest changes, even if they are minor aesthetic ones! Also one question about the Finished posteriors line, @Rick-Methot-NOAA ...should that be showing up for a MLE run? |
|
correct that significant output occurs in get_posteriors even in a MLE run. Thanks for checking. |
To eliminate conflicts in order to merge
|
@Rick-Methot-NOAA I noticed that this pull request is still marked as a draft request. I wanted to make sure that it is still needed/wanted before I tried to fix the merge conflicts. |
|
@kellijohnson-NOAA Yes, we were just waiting until using admb 13 until merging in! So I think fixing merge conflicts would be helpful, but it could also be done later. |
|
I have changes in these files queued in the PR for D-M and Tweedie. Let me merge that one first. Great if both of you could take a look at it first. Ian reviewed it last week. |
|
in progress. @nschindler-noaa is resolving merge conflicts |
|
All conflicts resolved. Just a matter of beginning/ending reading file comments and new message system (and new messages at end of run)). |
|
Excellent. Thanks Neal. |
| if (N_nudata > 0) | ||
| { | ||
| cout << "Creating bootstrap files: " << N_nudata << " files"; | ||
| cout << "Begin writing *.ss_new output files... "; |
There was a problem hiding this comment.
Space after files.
|
|
||
| // SS_Label_Info_12.4.5 #Call fxn write_nucontrol() to produce control.ss_new | ||
| write_nucontrol(); | ||
| cout << "done" << endl; // finish writing *.ss_new output files |
There was a problem hiding this comment.
I suggest a more verbose statement such as
"Finished writing *.ss_new output files"
| } | ||
| if (do_once == 1) | ||
| cout << " did lencomp obj_fun " << length_like_tot << endl; | ||
| echoinput << "Finished lencomp obj_fun " << length_like_tot << endl; |
There was a problem hiding this comment.
Not sure that it matters but there are two spaces after "obj_fun".
| ad_comm::adprogram_name = "ss"; | ||
| echoinput << "now in PARAMETER_SECTION " << endl; | ||
| echoinput << "Begin setting up parameters" << endl; | ||
| cout << "Setting up parameters... "; |
There was a problem hiding this comment.
Why are these statements on line 20 and 21 different? If you keep line 21 as is, I think there needs to be a space after "parameters" before the l-dots. APA style-guide says to treat the three dots as a word. This should be carried out throughout this pull request.
| // SS_Label_Info_6.8.1 #Call fxn get_MGsetup() to copy MGparms to working array and applies time-varying factors | ||
| get_MGsetup(styr); | ||
| echoinput << " did MG setup" << endl; | ||
| echoinput << "Finished MG setup" << endl; |
There was a problem hiding this comment.
Earlier you used "MGsetup" to better match the code base.
|
@Rick-Methot-NOAA , is it okay to merge even with more warnings? |
|
Can you find and send me the warnings file so we can review it before merging with the increased N warnings |
Places spaces before/after and after ... to treat it as if it is a word in its own right.
Nitpicky change to reduce spacing from 2 to 1 at end of line for consistency.
Changes done to be more explicit.
Make the lines essentially match in what is printed to the screen versus what is saved in echoinput
MG setup is changed to MGsetup which matches what the codebase is to help users that are searching for parameter names.
|
@Rick-Methot-NOAA I resolved all of the comments that I made with the last 7 commits. I think this is ready for your review because if my memory serves me correctly the first failing job will potentially change with the use of ADMB 13.0 and or the number of warnings expected needs to be changed. |
|
@kellijohnson-NOAA that is correct re the warnings job |
Rick-Methot-NOAA
left a comment
There was a problem hiding this comment.
Thanks Kathryn, Neal and Kelli. Looks great. Approving now.
What issue(s) does this PR address? Describe and add issue numbers, if applicable.
Link issue(s) here:
#313 . The new ADMB console (#257) inspired me to make some changes to the SS3 console output
What tests have been done? Upload any model input files created for testing in a zip file, if possible.
Some of the changes have been tested locally by compiling and running a model
What tests/review still need to be done? Who can do it, and by when is it needed (ideally)?
When the last changes are made, the output should be looked at in github actions to make sure everything looks ok
Check which is true. This PR requires:
Describe any changes in r4ss/SS3 manual/SSI/change log that are needed (if not checked):
May need to check on r4ss and SSI
Additional information (optional):