Nil selected fish warnings

[k-doering-NOAA]

Imported from redmine, Issue #70635 Opened by @k-doering-NOAA on 2019-10-30 Status when imported: In Progress

Huihua Lee discovered this issue while converting the Pacific bluefin tuna assessment from SS 3.24f to SS3.30.14. She found that there were some fatal errors, which Huihua was able to get rid of by making manual changes to the 3.24 model, but she still found that the fits to the data were different and there were many warnings in the SS3.30 version of the model. These warnings were of the form:

warn just once for:  Observation exists, but nil selected fish for year, seas, fleet 1969 4 21

Ian noted that there should have been selected fish available despite this warning.

Rick thought the difference in results between the models may be due to these warnings:

The check for "nil selected fish" does not occur in 3.24, so no warning is created there.

The warning was created in 3.30 because sometimes the selected fish got to 0 and the model would crash.

Looking at the 3.30 code, I have some misgiving regarding how this is implemented.
        if(sum(exp_l_temp)<1.0e-8)
          {
            if(do_once==1) {N_warn++; warning<<"warn just once for:  Observation exists, but nil selected fish for year, seas, fleet "<
Rick plans to look into this after the CAPAM workshop.
            

[k-doering-NOAA]

comment from @k-doering-NOAA on 2019-11-27: Some additional details gleaned from email chains:

@richard.methot originally marked this issue resolved after making the following changes to the code:

In 3.30, I couple the flag with a small addition to the N fish for that obs, but I now see that could interfere with the gradient, 
so I have reduced the size of that addition to a very tiny value.  See below.  However, this addition, if it is even necessary, is 
not being added to other quantities that support expected values, so I remain dubious regarding its merit.  While recognizing that
occasionally hitting this condition could be a source of NAN calculations.
in code below, exp_l_temp is the sum of the selected age-length key to the length axis.

        if(sum(exp_l_temp)<1.0e-8)
          {
           if(do_once==1) {N_warn++; warning<
@ian.taylor did some additional testing on this. In an email, he said:
I just played around with Hui-Hua's model a little bit trying to figure out why this warning was occurring when it seemed
 from the estimated selectivity and numbers at age that there should be a reasonable amount of selected fish.

In the process I ran the model using a .par file from a previously converged state and the warning no longer appeared. This 
made me realize that the warning gets printed only at the start of the estimation process, not at the end. So a combination 
of bad initial values for log(R0) or the selectivity parameters could create this warning, even if the problem is eliminated
 in the second phase. Indeed, starting at a high R0 without the par file also made the warnings go away.

Would it be possible to substitute the "if(do_once==1)" check for something that only happens once in the final phase?

Adding the constant doesn't bother me now that I know these messages were not associated with the final model estimates. As 
with the crash penalty, if it helps the model find a better solution where this penalty doesn't occur, then the somewhat 
arbitrary specifications of the penalty aren't that important.

@richard.methot may still make some additional changes to dealing with this issue, so I am changing its status back to "in progress". Rick thinks there are 3 possible ways of dealing with this:
The options I see are:

1.  remove warning and the addition - and hope nobody's convergence really depended on it
2.  move warning to last phase - but then it might show up repeatedly
3.  remove warning and apply the addition of nil fish to every cell of the sampled ALK - but this would warrant some
 performance testing
            

[k-doering-NOAA]

comment from @k-doering-NOAA on 2019-11-27: @richard.methot, some thoughts about which road to go down. I think ideally a warning should be generated ONLY if it means someone should scrutinize their final results because SS is behaving differently than they would expect it to and it can give them advice how how to troubleshoot their model. Warnings that can be safely ignored by the user shouldn't be generated, as they can lead to confusion (like in Huihua's model). Ideally, if a warning is needed, it would only be generated once, although I understand this could be difficult to do.

In its current form, I don't find the "nil selected fish" warning helpful, because it doesn't tell the user what action should be taken next. I suggest if you deem the warning necessary that it provide additional advice to the user on what steps to take next.

One other thought: I don't think SS should intentionally not warn for something that we know is an issue, also. I expect eventually it will affect someone's model, they will contact us, and we will forget all about the problem (as recently happened with Lorenzen M).

[k-doering-NOAA]

comment from @RickMethot on 2019-11-30: Good thoughts. On the first one, some warnings are actually user tips. So 3 categories of warnings:

1. fatal problems that are caught by the SS code and a warning message provided before exiting;
2. issues that could be problematic and bear closer attention by the user; hence a normal warning
3. factors for which an alternative setup is advised, such as the one for simpler recruitment distribution.

Another option would be to (a) remove warning; (b) always add the small constant until SS gets to last_phase, or sd_phase, or MCEVAL phase. Seems a good idea to leave it on for MCMC phase. Again, this will take more testing to be confident it works well and is actually an improvement. Problem is that we do not have an identified test case in which SS crashes because this feature is not there.

On Wed, Nov 27, 2019 at 10:35 AM vlab.redmine@noaa.gov wrote:

[k-doering-NOAA]

comment from @k-doering-NOAA on 2019-12-02: Those categories of warnings make sense to me!

Maybe @ian.taylor will have thoughts on which way to go when he gets back from leave on 3 Dec? It seems to me there isn't a rush to fix this issue, given that Huihua's model's warnings can be safely ignored (from what I can tell based on Ian's testing.)

[k-doering-NOAA]

comment from @k-doering-NOAA on 2020-01-22: I see commit:b164ea7f references this issue - @richard.methot, is this still in progress or has it been resolved?

And is a priority of 3 - High still appropriate for this issue, if it is not completed?

[k-doering-NOAA]

comment from @RickMethot on 2020-01-23: please change to a low priority. There is no burning issue here, but better warnings still a good idea.

On Wed, Jan 22, 2020 at 1:04 PM vlab.redmine@noaa.gov wrote:

[k-doering-NOAA]

comment from @iantaylor-NOAA on 2020-01-23: When we have more time to think about this issue (which I agree is low priority), we might start with trying to make a test case that actually benefits from an additional constant. If we can't create one, it's probably better to make the warning and the addition go away.

[Rick-Methot-NOAA]

revise warning to: if(sum(exp_l_temp)<1.0e-8) { if(do_once==1) {N_warn++; warning<<N_warn<<" warn in first call: Nil selected fish for year, seas, fleet "<<y<<" "<<s<<" "<<f<<"; SS may recover; suggest revising initial parm. values for selectivity and growth"<<endl;} exp_l_temp+=1.0e-09; }

Note the constant is only added if the warning is triggered. Warning now includes a suggestion Closing this issue.

[Rick-Methot-NOAA]

revised statement: the warning is only generated once, the added constant is triggered whenever the nil fish condition is met.