Add pred m to z not m

[Rick-Methot-NOAA]

What issue(s) does this PR address? Describe and add issue numbers, if applicable.

Link issue(s) here: #88 and #235

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

extensive look at all test models, special testing using the predator model: vendance.

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

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/change log that are needed (if not checked):

natural mortality report table has been reformatted
technical description of these changes is in attached doc
natM tech doc.txt

Additional information (optional):

[k-doering-NOAA]

Thanks @Rick-Methot-NOAA for all this work! It looks like a ton of restructuring was necessary for this feature, so it would be good to do thorough testing before merging into main. Could you share with me a tester model that uses predation so I can test?

I took a look through the code and noted mainly 1) where style needs to be made consistent; 2) where comments, output to users, and variable names are unclear; 3) questions about if the statement is correct. I'll admit some of the comments are picky, but I think given our recent efforts to make style more consistent in SS3, it would be helpful to not introduce inconsistent style to the codebase again. Hopefully in the future the styling exe will become good enough that we could feel comfortable just running it before PRs to fix any style changes, but I don't think it is there yet.

I still couldn't tell very well to what extent changes make sense; maybe @iantaylor-NOAA could provide feedback on that deeper aspect.

[k-doering-NOAA]

@Rick-Methot-NOAA , sorry, I noted after I did this review that @kellijohnson-NOAA was included as the assignee! Maybe in the future we should just request reviews instead of adding an assignee? I'm not sure assignee makes sense with our workflow, but it doesn't indicate to me that a review is wanted from this person...

[Rick-Methot-NOAA]

Thanks Kathryn for that thorough review. It will take me a while to go through all of them, so I'll start with overall response:

1. white space style. It is very difficult to remember to put space around everything. This is something that we should rely on the styler to add later.
2. missing {} - agree that code is more explicit if they are always used, but in many instances the compiler does not need them. Much of what you noted was already there.
3. variable names. I tried to lay out the logic in the text document. natM_M1(s is calculated in biofxn and produces an ephemeral version of age-specific M that is immediately transferred into natM(t. Then the overall level of predation for each predator is pred_M2. So pred_M2 for predators is equivalent to Hrate for fishing fleets. Neither pred_M2 or Hrate are age-specific and only inherit age-specificity when used in conjunction with the selectivity of that fleet or predator. Age-specific predation gets added onto natM to get the total M (e.g. M1+m2). Then Zrate is age-specific and based on natM + sum(Hrate*selec)
4. reviewers - I added Kelli to bring this to her attention, but I agree with you for our team style specific reviewers are less necessary. However, having a reviewer named does make it more clear that someone needs to review before merging.
5. "special_flag" is for my special purposes while developing. Leave it in else I need to create it anew. I use it to do debugging write statements under particular situations.

[kellijohnson-NOAA]

I have this on my task list for today, though I don't know if I will be able to finish it b/c it might take me a while to get up to speed with all of the changes.

[Rick-Methot-NOAA]

here is an example that includes a predator in a 4 season empirical wt-at-age model.
predator.zip

[k-doering-NOAA]

Thanks Kathryn for that thorough review. It will take me a while to go through all of them, so I'll start with overall response:

1. white space style. It is very difficult to remember to put space around everything. This is something that we should rely on the styler to add later.
2. missing {} - agree that code is more explicit if they are always used, but in many instances the compiler does not need them. Much of what you noted was already there.
3. variable names. I tried to lay out the logic in the text document. natM_M1(s is calculated in biofxn and produces an ephemeral version of age-specific M that is immediately transferred into natM(t. Then the overall level of predation for each predator is pred_M2. So pred_M2 for predators is equivalent to Hrate for fishing fleets. Neither pred_M2 or Hrate are age-specific and only inherit age-specificity when used in conjunction with the selectivity of that fleet or predator. Age-specific predation gets added onto natM to get the total M (e.g. M1+m2). Then Zrate is age-specific and based on natM + sum(Hrate*selec)
4. reviewers - I added Kelli to bring this to her attention, but I agree with you for our team style specific reviewers are less necessary. However, having a reviewer named does make it more clear that someone needs to review before merging.
5. "special_flag" is for my special purposes while developing. Leave it in else I need to create it anew. I use it to do debugging write statements under particular situations.

Hi, Rick, thanks for all these responses! Certainly, feel free to not address comments I made that you don't agree with, as I don't want perfect to be the enemy of good. A few things, though:

1. I agree with using the styling tool to fix whitespace, but given it still isn't totally automated, I think it would be good to avoid introducing poor styling into main when possible. I can help make these changes, but I thought it might be an opportunity to help you remember the whitespace rules better? If you don't see it that way, though, I understand!
2. Adding in the {} makes the code more readable, even though the compiler doesn't need them. I can make these changes, if you would like. I think as we work on code, if we run into bad style we should fix it on the lines being modified?
3. Thanks for the tech document!
4. I see - Perhaps discussing how we use the fields on PRs would be something worth checking on.
5. Ah, got it! Maybe a comment where it is defined could make this clear?

[Rick-Methot-NOAA]

Most recommended changes were implemented.
array natM_M1 has been removed. Now, M1 is stored directly into the 0'th row of natM(). Updated tech doc attached here.
natM tech doc.txt

[Rick-Methot-NOAA]

@k-doering-NOAA - I hope I addressed your review requests adequately. Did you get the vendance example for testing?

[k-doering-NOAA]

Thanks @Rick-Methot-NOAA , I will take look by Wed at the very latest. I see the example files, thank you!

[k-doering-NOAA]

@Rick-Methot-NOAA , did the changes get pushed up to github? I'm not seeing new commits...

[Rick-Methot-NOAA]

• @Rick-Methot-NOAA. One additional test that should be done is to create a multi-area example with a predator acting in just one area. Then examine output to verify that pred_M2 is only being added to that area and not to the other areas.

[k-doering-NOAA]

Good call @Rick-Methot-NOAA!

I've been prioritizing the admb 13 testing over this, but I can course correct and work on reviewing this issue sooner, if needed. The code looks good, but I still wanted to do some model runs.

[Rick-Methot-NOAA]

K - stick with the console project. I can create a suitable test for area-predators.

[k-doering-NOAA]

@Rick-Methot-NOAA , if you can share the area-predators test you create, that would be great! If it runs fast (preferably in <10 min), I think we should add it to our test set when this branch is merged into main so we continue testing the feature.

[Rick-Methot-NOAA]

I modified the vendance example to have areas, then assigned the predator to either area 1 or area 2. The results confirm that predator-by-area is working as expected. I believe this PR is ready to be merged into main.
predator_area.zip
Note that one of the recent commits has created a conflict in ss_selex.tpl

[k-doering-NOAA]

Thanks @Rick-Methot-NOAA! Luckily this merge conflict was pretty easy to resolve.

@kellijohnson-NOAA, do you still think we should set up a test to compare using predM with using the old way as just a fishing fleet and making sure the models should still get the same Z? Or would you support merging it into main given the spatial example Rick has created?

[Rick-Methot-NOAA]

The test of predator as fishing fleet was done when predator fleet was first created last fall with 3.30.18. This new commit makes it area-specific, but in doing so it needed to change internal indexing of natM. Since that internal indexing change has not perturbed anything else in the model, I hope we are good to go.

[k-doering-NOAA]

Sounds good @Rick-Methot-NOAA ! I approved the changes, so if you are ready merge into main, go ahead.