Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

191 spawner recruitment with a b formulation #442

Closed

Conversation

Rick-Methot-NOAA
Copy link
Collaborator

@Rick-Methot-NOAA Rick-Methot-NOAA commented May 17, 2023

Concisely (20 words or less) describe the issue

create new spawner-recruitment with a,b formulation

Please Link issue(s)

#191
resolves #191 #341 #625

What tests have been done? Upload any model input files created for testing in a zip file, if possible.

produces exact match to SRR=3 when tested without time-varying biology.

What tests/review still need to be done? Who can do it, and by when is it needed (ideally)?

Need to test with time-varying biology then add more output to show consequences of implied steepness changing over time.

Has any new code been documented?

  • I have documented any new code added (or no new code was added)

is there an input change for users to Stock Synthesis?

  • Yes, there was an input change

If so, please provide an example of the new inputs needed.

[New example stock synthesis input goes here]

Check which is true. This PR requires:

  • no further changes to r4ss
  • no further changes to the manual
  • no further changes to SSI (the SS3 GUI)
  • no further changes to the stock synthesis change log (new features, bug reports)

Describe any changes in r4ss/SS3 manual/SSI that are needed (if not checked):

If changes are needed in the change log, please fill in the table here:

Action Topics Type
[fix, new, or revise] [e.g., biology. Use issue label options.] [input, output, and/or calc, or ALL]

Additional information (optional):

@Rick-Methot-NOAA
Copy link
Collaborator Author

Rick-Methot-NOAA commented May 20, 2023

create example with time-vary growth (Linf increase by 5 cm). See impact on SPB/R in expanded output to SPAWN_RECR table; but very little impact on estimated recruitments. Implied steepness move from 0.6 to 0.63 with the change in SPB/R.
Note that r4ss will be broken for now. Need to cleanup the SPAWN_RECR table first.
@timjmiller/wham
compare_SRR.zip

@Rick-Methot-NOAA
Copy link
Collaborator Author

example file here and in the Issue.
update_SRR_AB.zip

@iantaylor-NOAA
Copy link
Contributor

To use r4ss with the SS3 version in this 191-spawner-recruitment-with-a-b-formulation branch, it necessary to install the expanded_SR branch of r4ss via
devtools::install_github("r4ss/r4ss@expanded_SR")
or
pak::pak("r4ss/r4ss@expanded_SR")

@iantaylor-NOAA
Copy link
Contributor

Two notes on failing github actions:

  1. "build-ss3 / macos-12" github action can be ignored as discussed in Anyone still using Mac OS 12? Deprecate the build that's failing? #629. The action has been updated in main but this branch is behind and hasn't gotten the update to use macos-13 instead of macos-12.
  2. The "run-ss3-no-est" action is failing because three models (growth_timevary, Simple_with_DM_sizefreq, and two_morph_seas_areas) have this error: Fatal Error! user must select 1, 2, or 3 for updating SPR0 flag (formerly labelled future feature in SR input) because there is time-varying biology.

@Rick-Methot-NOAA do you have a preference on the SPR0 flag used in the test models? I'm not yet familiar with the options and don't know how much difference any would make for the test models.

@Rick-Methot-NOAA
Copy link
Collaborator Author

Ian,
when there is time-varying biology, the model can use option 2 to maintain backward compatibility with what seems to be an incorrect approach. We can convert examples the the correct option 1 approach later.

iantaylor-NOAA added a commit to nmfs-ost/ss3-test-models that referenced this pull request Oct 21, 2024
@iantaylor-NOAA
Copy link
Contributor

Thanks @Rick-Methot-NOAA.
The "run-ss3-no-est" action is now passing. I accidentally set the values to 1 ("correct") for all three models in my first commit, which was fine for two of the three models, but impacted the ForeCatch values in the growth_timevary model enough to fail the test. Changing the setting to 2 ("backwards compatibility") for just the one model got the test passing.

@Rick-Methot-NOAA
Copy link
Collaborator Author

@e-perl-NOAA

  • Can you update this branch with the 3.30.23 main?

@e-perl-NOAA e-perl-NOAA force-pushed the 191-spawner-recruitment-with-a-b-formulation branch from 95be4a1 to 27d4095 Compare October 31, 2024 20:53
@e-perl-NOAA
Copy link
Collaborator

@Rick-Methot-NOAA please review to make sure that everything was implemented in the rebase correctly. For everything but the updated ss3-build.yml workflow I went with the changes that you made in this branch when merge conflicts occurred.

@e-perl-NOAA
Copy link
Collaborator

After going through the rebase I keep getting this error on the build:
Error in line 13129 while reading
L
Error in line 3983 while reading
*
@Rick-Methot-NOAA @iantaylor-NOAA could either of you help decipher where that error is coming from?

@Rick-Methot-NOAA
Copy link
Collaborator Author

that error must be me. Let me s33 if the ab branch compiles locally. Possible that I had not pushed a fix.

@e-perl-NOAA
Copy link
Collaborator

e-perl-NOAA commented Oct 31, 2024

This branch has been open for a while which made the rebase challenging so it might have been an error introduced there. I should have rebased this branch earlier in the process and rebased it frequently throughout but I didn't know how long this was going to take initially.

@Rick-Methot-NOAA
Copy link
Collaborator Author

OK. It does compile locally. VS Code says that there are 40 incoming changes from origin and 30 outgoing changes.

e-perl-NOAA pushed a commit to nmfs-ost/ss3-test-models that referenced this pull request Oct 31, 2024
@Rick-Methot-NOAA
Copy link
Collaborator Author

Locally, I pulled in the 40 changes and saved to a new branch. That branch fails to compile because in the ss_param.tpl file there are numerous lines that have lost the required initial indent.
Perhaps rather than do the rebase, I should try to merge the #191 branch into main locally while fixing any conflicts, then continue with that as a new version of the work. I'll await your advice next week.

@Rick-Methot-NOAA
Copy link
Collaborator Author

Rick-Methot-NOAA commented Nov 1, 2024

Well. I did a pull(rebase) and now my local is messed up also. There are many lines where the initial 2 spaces are missing. In SS_param.tpl is seems to occur right after clang-format on. Then elsewhere I see that the {} than delineate a FUNCTION is not indented.
@iantaylor-NOAA please save your local version of this branch just in case recovering from there is easier than fixing the rebase.

@iantaylor-NOAA
Copy link
Contributor

Sorry about the troubles. I won't touch my local version. I'm guessing that someone more skilled with rebasing or merging could help us sort this out next week. The git history should also all be recoverable from github.

@e-perl-NOAA
Copy link
Collaborator

@iantaylor-NOAA can you make a branch off your untouched local version (maybe even 2 just to have an extra around in case we need it) and publish that, then rick and I can go through the rebase together?

@iantaylor-NOAA
Copy link
Contributor

I just pushed the 191-spawner-recruitment-backup branch. It does not include the "latest update" commit 27d4095 but that should be too hard to add back manually if necessary.

@Rick-Methot-NOAA
Copy link
Collaborator Author

OK. I have your backup locally and verified that it compiles. I also see this branch with "cherrypick" and don't know what to do with that.

@e-perl-NOAA
Copy link
Collaborator

I just deleted it. Any branch you see created after Ian's just ignore. I'm troubleshooting ways to try to merge things. Ultimately Rick, you and I are going to need to go through certain merge conflicts together because with many of them, you will know what the code should best look like. I'll schedule that when I figure out the best way to proceed.

@Rick-Methot-NOAA
Copy link
Collaborator Author

switching to rebased branch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants