new_filename_and_folder_for_outputs#218
Conversation
|
@Rick-Methot-NOAA , this is to address #70. right? |
|
Also, I should have said that the failing run was due to an issue with archiving the exe, which looks like is a gh actions issue, not related to the stock synthesis code. |
|
I tested out this change locally, and it worked a little differently than I expected.
Some potential changes to consider, depending what the scope of this feature is:
|
|
Thanks Kathryn for the feedback. This is definitely a WIP so I am not surprised that it looks incomplete and it is a long way from merging. I did pull request just to get it into the system and should have noted then that it is not expected to run completely yet.
|
|
Thanks for all the commentary, Rick, it was helpful on getting me up to speed. As for the data.ss_new split, I'm not sure about it, but I think it is just because I am used to the old way. It would require people to change their workflow, but then again it wouldn't be that hard to deal with. It is easy to accidentally look at the wrong section of data.ss_new (I have done this many times when just scrolling through the text files), so I see the advantage. I'd be curious to hear @kellijohnson-NOAA and @chantelwetzel-noaa 's thoughts as well. |
|
The protocol is now working for all ssnew and all sso files. I imagine a significant augmentation would allow user to input the name of the output folder in starter.sso, then SS3 would create that folder at runtime if it did not exist. |
|
Kelli, Chantel and Ian - could I get you opinion on what to do with the data.ss_new while I work to get report for each mceval working. Right now the branch does (b) and (d) |
|
(a) - lots of workflows depend on it |
|
I think (d) would be my ideal, but in the simulations I've worked on (long ago), I wanted to the expected recdev time series for each bootstrap so never wrote more than 1 per model run in which case (c) and (d) are essentially the same. I know that past workflows have all used the status-quo approach but don't have a good sense of how many of those are likely to ever be repeated. Adapting to any of these options also shouldn't be very hard, so I'm not sure that backward compatibility is important if it adds complexity to the code. As for run time, I would guess that the bootstrap and file-writing process is much faster than the model estimation step that is surely part of almost all bootstrap analyses so it's not worth trying to hard to optimize the run time. |
|
Lets clarify what we mean by bootstrap. There is the bootstrap process
inside of SS3 that is post-estimation and the only slow part of it is doing
the multinomial sampling for all the comps. We are talking about the
output from that process. Then there is the ss3sim process that repeatedly
calls the entire ss3 process, that takes longer and each of the ss3sim
calls creates the entire set of ss3 outputs.
…On Wed, Nov 10, 2021 at 1:35 PM Ian Taylor ***@***.***> wrote:
I think (d) would be my ideal, but in the simulations I've worked on (long
ago), I wanted to the expected recdev time series for each bootstrap so
never wrote more than 1 per model run in which case (c) and (d) are
essentially the same.
For (c) and (d), I like the filenames proposed by @kellijohnson-NOAA
<https://github.com/kellijohnson-NOAA> in the comment above #218 (comment)
<#218 (comment)>
.
I know that past workflows have all used the status-quo approach but don't
have a good sense of how many of those are likely to ever be repeated.
Adapting to any of these options also shouldn't be very hard, so I'm not
sure that backward compatibility is important if it adds complexity to the
code.
As for run time, I would guess that the bootstrap and file-writing process
is much faster than the model estimation step that is surely part of almost
all bootstrap analyses so it's not worth trying to hard to optimize the run
time.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#218 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABPV4IA27D6H56IVINYQYYLULLQQVANCNFSM5GPK63LA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
|
I generally agree with Ian and Kelli's suggestions. I would prefer option
c or d with the naming structure proposed by Kelli for clarity. Similar to
Ian, I have only done simulations where I generate a single bootstrap at a
time, so don't have much of an opinion on generating all bootstraps in a
single file or generating separate files.
Chantel Wetzel, PhD
|
|
We will let this one sit for a bit, as we don't want to change the subfolder and file structure in the main branch yet. However, we plan to merge in before the 3.30.19 release. |
|
Kelli, @kellijohnson-NOAA |
|
I am testing this out today. |
|
@Rick-Methot-NOAA is the functionality intentional to have posteriors.sso be a blank file? Also, as written the system works when I run it with standard MCMC but @cgrandin was having trouble when using @Cole-Monnahan-NOAA's adnuts. Chris is looking into this more as will I. |
|
Nothing intentional was done to change posteriors, so I will check on that.
No idea how adnuts would be affected.
…On Tue, Dec 14, 2021 at 3:53 PM Kelli Johnson ***@***.***> wrote:
@Rick-Methot-NOAA <https://github.com/Rick-Methot-NOAA> is the
functionality intentional to have posteriors.sso be a blank file? Also, as
written the system works when I run it with standard MCMC but @cgrandin
<https://github.com/cgrandin> was having trouble when using
@Cole-Monnahan-NOAA <https://github.com/Cole-Monnahan-NOAA>'s adnuts.
Chris is looking into this more as will I.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#218 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABPV4IGZNEZ37PUL62SQ5WDUQ7KIJANCNFSM5GPK63LA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
|
@kellijohnson-NOAA code is now writing correctly to posteriors.sso. I do not know how to investigate the adnuts issue. |
|
Great. I will distribute the new executable to the team. I checked with Chris and he said that it ended up being fine with adnuts. I think he panicked because the files are not written until the end. Sorry for the false alarm there. |
|
@k-doering-NOAA Please merge this into main after you are able to adjust the workflow to deal with the new filenames. |
|
Ok, sounds good, Rick. I'm working on this now; hope to finish by end of the day monday at the latest! |
|
Hi all, Thanks for getting this done and so quickly! Thanks |
|
Hi Chris.
That would be possible, but it seems overly complicated to have some sso
files in the main folder and others in the sso folder. Do you have a
complete list of which you would want where?
All sso files go to the main folder if the sso folder has not been created
by the user prior to the ss3 run starting.
Rick
…On Fri, Dec 17, 2021 at 7:50 PM Chris Grandin ***@***.***> wrote:
Hi all,
Thanks for getting this done and so quickly!
One little thing though is that the posteriors.sso, deriver_posteriors.sso,
etc files are located in the sso folder. This may break some other folks
code if they try to run with an sso folder and have current code that
reads those files from the parent mcmc folder. Would it be possible to
move those files back to the parent directory of sso?
Thanks
—
Reply to this email directly, view it on GitHub
<#218 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABPV4IGEUKLV75HSHKDBJUDURQAIDANCNFSM5GPK63LA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
Some observations on this PR:
|
|
I think it's totally fine, we have everything working the way it is. Thanks! |
|
@Rick-Methot-NOAA I just noticed that the option 2 for MCMC detail is not yet described in starter.ss_new comments: |
|
I will fix that. |
|
In my final push on this issue I revised the numbered file names to have leading digits. |
SS_write_report.tpl
Outdated
| SS2out<<" end selex output "<<endl; | ||
| } // end do report detail | ||
| wrote_bigreport++; | ||
| SS2out.close(); |
There was a problem hiding this comment.
@Rick-Methot-NOAA For some reason this line leads to Dynamic Bzero NOT being written in Report.sso. At the top of the report file it says Y Dynamic_Bzero report:59 but the section is not written.
There was a problem hiding this comment.
it should be in report.sso.
I intentionally turned off the profiling outputs for the MCEVAL outputs but did not intentionally change dynamic Bzero. I will chenck on this also
|
Thanks Rick, I checked and was able to successfully get 1002 samples ✔️. One last mystery that is only happening in MCMC mode with this executable is in |
|
are you seeing that discrepancy in report.sso, or in one of the mce reports? |
|
@Rick-Methot-NOAA the problem with sel*wt... happens in both Report.sso and Report_mce_xxxx.sso if running in with -mcmc. If I just run |
|
Kelli, |
|
Found the solution. Now I can relax |
|
Thanks Rick. I will look at this tomorrow. Merry Christmas 🎄 🎁 🎄 |
|
Glad to see all these new fixes! Congrats. I am rerunning the checks on this branch : https://github.com/nmfs-stock-synthesis/stock-synthesis/tree/out_to_sep_check, which merged into the main branch so the newer gh action files could be used (which is the reason for the failing tests on this branch). |
|
Unfortunately, I am not able to get the latest fix to work with the hake model that I am testing it on. If I run in MLE I get dynamic bzero but there are still no values even though I have it turned on when running in MCMC. As well as the final year for the |
|
The simple lorenzen model has a change in results when run with estimation, I think because the se for the forecast catch value decreased see these model results Rick, I think you remember you fixing something to do with forecast SE not too long ago, so I wonder if that is the reason? Perhaps the change is expected? |
|
Kelli - you need only make this change in starter: 2 # MCMC output detail: integer part (0=default; 1=adds obj func components; 2= +write_report_for_each_mceval); and decimal part (added to SR_LN(R0) on first call to mcmc) |
will write to /ssnew and /sso folders if they exist and should write separate report and compreport files in mceval (not tested).