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

convert catchfleet IF statements to lookup table #192

Closed
Rick-Methot-NOAA opened this issue Jul 9, 2021 · 7 comments · Fixed by #230
Closed

convert catchfleet IF statements to lookup table #192

Rick-Methot-NOAA opened this issue Jul 9, 2021 · 7 comments · Fixed by #230
Assignees
Labels
code tuneup improve code logic or execution speed wishlist request new feature; bigger than revision; OK to remove after adding to Milestone
Milestone

Comments

@Rick-Methot-NOAA
Copy link
Collaborator

code like:
for (g=1;g<=gmorph;g++)
if(use_morph(g)>0)

better as:
for (g1=1;g1<=gmorph_use;g1++)
{g=g_use(g1);
.......

similar for fishing fleets

@Rick-Methot-NOAA Rick-Methot-NOAA added wishlist request new feature; bigger than revision; OK to remove after adding to Milestone code tuneup improve code logic or execution speed labels Jul 9, 2021
@Rick-Methot-NOAA Rick-Methot-NOAA self-assigned this Jul 9, 2021
@Rick-Methot-NOAA
Copy link
Collaborator Author

Rick-Methot-NOAA commented Aug 17, 2021

for fishing fleets, I have started making changes using the following syntax:
A. in readdata, create a vector containing fleet number for each fishing fleet:
if(fleet_type(f)<=2) {
N_catchfleets++;
fish_fleet(N_catchfleets)=f; // to find the original fleet index
...........

Then anywhere we want to process only the fishing fleets:
for(int ff=1;ff<=N_catchfleets;ff++) {
f=fish_fleet(ff);
<any code that uses "f" >

This should be more efficient than:
for(f=1;f<=Nfleet;f++)
if(fleet_type(f)<=2){
...........

unfortunately, this cannot be done by a global search and replace.

It may be useful to have an additional index:
imatrix fish_fleet_area(1,pop,1,N_fleet)
because many of the F calculations are only done for the fleets in an area, so more efficient to only loop over those fleets.

@Rick-Methot-NOAA Rick-Methot-NOAA added this to the 3.30.19 release goal milestone Sep 30, 2021
@Rick-Methot-NOAA
Copy link
Collaborator Author

There are 105 instances of
if(use_morph(g)==1)
in the code.

My review of the logic beginning on line 356 of readcontrol.ss is that use_morph(g) is ALWAYS true.
use_morph(g) becomes true if:
if(recr_dist_pattern(gp,settle,p)==1)

which means that if a gp (morph) settles in any area (p), then it is "used".
If it is not used, then a warning should be issued.

I suspect use_morph(g) is a relic from 3.24 birthseason logic and that all 105 of those IF statements can be removed from SS, hopefully for some execution time speed-up.

Before I do so, can some of you review my logic.
@k-doering-NOAA
@kellijohnson-NOAA
@iantaylor-NOAA
@chantelwetzel-noaa

@Rick-Methot-NOAA Rick-Methot-NOAA added the in progress This is being worked on in a branch label Oct 8, 2021
@Rick-Methot-NOAA
Copy link
Collaborator Author

for the fleet IF statements the replacement protocol is:
if dealing with agnostic areas:
for(int ff=1;ff<=N_catchfleets(0);ff++)
{
f=fish_fleet_area(0,ff);

or for a specific area, p:
for(int ff=1;ff<=N_catchfleets(p);ff++)
{
f=fish_fleet_area(p,ff);

@iantaylor-NOAA
Copy link
Contributor

I took a quick glance through https://github.com/nmfs-stock-synthesis/stock-synthesis/blob/main/SS_readcontrol_330.tpl#L356-L391. I can't claim to understand all the different possibilities of birth season, settlement event, morph, growth-pattern, etc. but your explanation makes sense so I support you removing the if(use_morph(g)==1) statements and adding a warning somewhere conditioned on if(use_morph(g)!=1) or similar.

@Rick-Methot-NOAA
Copy link
Collaborator Author

create useobs_l(1,nfleet,1,nobs) with values 0 or 1 depending on header_l(f,i,3)
then
if(header_l(f,i,3)>0) length_like_tot(f)+=length_like(f,i)
can be replaced by vector multiply
length_like_tot(f)+=length_like(f)*use_obs(f);

@Rick-Methot-NOAA
Copy link
Collaborator Author

Rick-Methot-NOAA commented Oct 20, 2021

I have created a scenario in which there are some "g" indexes that are not used.
This input to a 2 area, 2 seas, 2 GP model:
3 # recr_dist_method for parameters: 2=main effects for GP, Area, Settle timing; 3=each Settle entity; 4=none (only when N_GPNsettlepop==1)
1 # not yet implemented; Future usage: Spawner-Recruitment: 1=global; 2=by area
3 # number of recruitment settlement assignments
0 # unused option
#GPattern month area age (for each settlement assignment)
1 1 1 0
2 1 2 0
2 2 2 0

produces this g indexing scheme (as shown in echoinput):
MORPH_INDEXING
g Sex GP Settlement Birth_Seas Platoon Platoon% SexGP SexGP*settle_time Used(0/1) SettleTime_frac_yr
1 1 1 1 1 1 100 1 1 1 0
2 1 1 2 1 1 100 1 2 0 0.0833333
3 1 2 1 1 1 100 2 3 1 0
4 1 2 2 1 1 100 2 4 1 0.0833333
5 2 1 1 1 1 100 3 5 1 0
6 2 1 2 1 1 100 3 6 0 0.0833333
7 2 2 1 1 1 100 4 7 1 0
8 2 2 2 1 1 100 4 8 1 0.0833333

Take-away:

  1. area does not matter, the indexing is just for biological entities which can be distributed across and move between areas.
  2. the "not used" occurs because morph 2 has 2 settlement times and morph 1 has only 1.
  3. this is a 3.24 relic in which recruitment distribution was by main effects. I should be able to make this go away if I can internally switch all the old inputs of main effects to the new recruitment distribution method 3. Right now, even if user selects method 3 for each settlement event, SS3 still creates the blank "g".
  4. Emphasizing that area does not matter: if the above example was changed to put both of morph 2's settlements in month 1 but separate areas, then SS3 only creates two biological settlement timings (for morphs 1 and 2) with morph 2's settlement split between areas.
  5. All of this was one of the most vexing parts of the 3.30 creation as I was late in fully grasping the difference between settlement timings and settlement areas. Timings require a "g", but areas just split a "g" among areas.
  6. I now see that this would have been way easier if I had created "3=each settlement assignment" when I created 3.30 and forced conversion at that time. I may look further into this, but suspect that the current indexing is so entwined into fabric of the code that cost/benefit may not be favorable. But I cannot pull plug on use_morph(g) without doing that conversion.

@Rick-Methot-NOAA
Copy link
Collaborator Author

I have completed transition of the fleet IF to Lookup on popdyn and benchmark.

I will merge into Main, then leave this issue open for now, but suspect that I will not get back to doing same for the gmorph.

@Rick-Methot-NOAA Rick-Methot-NOAA changed the title eliminate some IF statements convert catchfleet IF statements to lookup table Dec 1, 2021
@Rick-Methot-NOAA Rick-Methot-NOAA added resolved issue resolved, look for "needs test" label and removed in progress This is being worked on in a branch labels Dec 9, 2021
@Rick-Methot-NOAA Rick-Methot-NOAA linked a pull request Jan 26, 2022 that will close this issue
@k-doering-NOAA k-doering-NOAA removed the resolved issue resolved, look for "needs test" label label Apr 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code tuneup improve code logic or execution speed wishlist request new feature; bigger than revision; OK to remove after adding to Milestone
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants