pacific-hake / hake-assessment

:zap: :fish: Build the assessment document using latex and knitr
MIT License
13 stars 6 forks source link

Figure 34 (compare MLE to MCMC results) needs lots of fixing, as someone noted in caption #653

Closed andrew-edwards closed 4 years ago

andrew-edwards commented 4 years ago

This may be done already, I can't rebuild right now, so just mentioning it. Caption needs clarifying also.

andrew-edwards commented 4 years ago

Not fixed yet

kellijohnson-NOAA commented 4 years ago

There are warnings "no non-missing arguments to min ..." I can work on it.

andrew-edwards commented 4 years ago

Presume I don't need to update r4ss?

andrew-edwards commented 4 years ago

Works fine, thanks.

andrew-edwards commented 4 years ago

So Figure 34 has spawning biomass values about twice as big as they should be (B0 should be 1.385 (MLE) and 1.832 (median), or just compare with Figure c). Figure was correct last year - can't see that commit be1dc5c would have changed it. I can dig into this, but expect @kellijohnson-NOAA may have an initial idea ;)

andrew-edwards commented 4 years ago

Same problem occurs for retrospective plot Figure 54 top panel, for spawning biomass. Something to do with 'spawning biomass' changing definition somewhere in r4ss or SS?

andrew-edwards commented 4 years ago

Summarising texts to Kelli and Aaron - all plots showing spawning biomass comparisons seem to have about twice the correct biomass (not exactly twice though, I don't think). So bridging and sensitivity plots, plus the MLE vs MCMC one. in SSplotComparisons there's a warning that says to set nsexes=-1 in data file, which doesn't seem the best solution. Conclusions aren't (presumably) affected regarding different model results. Kelli took a look (thanks!). She thinks r4ss used to just divide by two and doesn't now. We won't get it fixed in time for the presentations, so can just say that the absolute scales of comparison figures are wrong, and we'll fix them in the assessment document. And that they were correct in past years (to allay any fears). Could change the ylab names, but a bit tedious and we'll get it fixed for future anyway.

aaronmberger-nwfsc commented 4 years ago

I had no idea about this change - but I'm glad we found it before looking dumbfounded during the meeting. I will rerun all the MLE's for the models that we use in the document and upload them to the google drive under models_update. It doesn't look like we need to redo MCMCs, for whatever reason they seem correct (even though hand calculating the median 2020 SSB from nuisance posterior file show that it is incorrectly 2x there also, it somewhere get calculated correctly!?!).

aaronmberger-nwfsc commented 4 years ago

I actually don't think we will need to do anything else but re-build the Rdata files and the document.

cgrandin commented 4 years ago

For what it's worth, I tried setting base.model$nsexes <- -1 and re-running the plot code but it had no effect ( this is the short-cut to do what you're talking about)

cgrandin commented 4 years ago
make.mcmc.vs.mle.plot(base.model,
                      end.yr = end.yr,
                      type = "o",
                      subplot = 2)
base.model$nsexes <- -1
make.mcmc.vs.mle.plot(base.model,
                      end.yr = end.yr,
                      type = "o",
                      subplot = 2)

Are the same

andrew-edwards commented 4 years ago

I think it's best if we can change r4ss. Better than rerunning all models, in case it changes other things? I guess we can double check after. Can discuss tomorrow.... @aaronmberger-nwfsc - Fig 34 has the MCMC results wrong also. So we really don't want to go down the route of rerunning everything...

aaronmberger-nwfsc commented 4 years ago

Maybe we could get a separate branch of r4ss just for this assessment and then make a note to make that change when building models next year. Good idea Andy...

aaronmberger-nwfsc commented 4 years ago

Just as a test a re-ran a few models and then did the first bridge model - though note that the -1 for nsexes in the data file doesn't work in last years base, so that leaves a weird feature. If we got an altered r4ss to use it would fix this too. compare2_spawnbio_uncertainty

cgrandin commented 4 years ago

The actual report file is the problem The values reported in there are what's being plotted. So nothing changed in r4ss to make this happen. I think we need to re-run the models

cgrandin commented 4 years ago

Just as a test a re-ran a few models and then did the first bridge model - though note that the -1 for nsexes in the data file doesn't work in last years base, so that leaves a weird feature. If we got an altered r4ss to use it would fix this too. compare2_spawnbio_uncertainty

We could just divide the biomass by 2 for last year's model. The numbers are exactly half.

andrew-edwards commented 4 years ago

Morning....

So shall we agree to not change anything for the talks? I'd rather spend the time checking what I'm going to say, and from. We can just explain that:

andrew-edwards commented 4 years ago

I think my earlier comment about values not being exactly double is wrong. The MLE and MCMC estimates of B_0 are different enough (1.38 v 1.83 million tonnes), that 2B_0 is 2.76 v 3.66. Quite a difference (when tickmarks are every integer), but down to MLE v MCMC.

aaronmberger-nwfsc commented 4 years ago

No need to fix this week as it doesn't matter in the slightest for reviewing what we've done. All we are asking is that they ignore the SSB y-axis for SS plot comparisons in bridging and sensitivities (and MLE vs MCMC) b/c it represents 2x SSB (or not multiplied by fraction female (i.e. 0.5).

aaronmberger-nwfsc commented 4 years ago

I mean WE can fix it during the week for ourselves (I'll be running models during the day from time to time). I'm guessing we could have a new document done by later this week, but I still don't see a need to have that reviewed, compared to what we already have.

cgrandin commented 4 years ago

Since I have the first bridging plot in the assessment talk which has this issue, I thought it would be a good time to stop and mention the issue. It's near the beginning and I can explain that it is for all MLE plots including bridging, sensitivities, retrospectives, and MLE/MCMC base plots.

I opened a new issue to deal explicitly with Figure 34, as the MCMC is also double for some reason on that plot. That seems to be a different issue from the MLE doubling.

kellijohnson-NOAA commented 4 years ago

There is a lot of other "stuff" in the hake-assessment package that is going to break if you rerun all of the models with sex == -1. I recommend that we just change SSplotComparisons back to what it was (working on that right now) and use a branch. Ian wants to have a bioscale argument any way, so it will be helpful to him and us.

kellijohnson-NOAA commented 4 years ago

This is what we want right? image

cgrandin commented 4 years ago

Yes! How did you do it?

kellijohnson-NOAA commented 4 years ago

Install

devtools::install_github("r4ss/r4ss@hake2020")

and it should work. All I did was undo Ian's two commits that took out the hard-wired /2 calls.

cgrandin commented 4 years ago

Should we do this fix for today's talks? Asking everyone..

aaronmberger-nwfsc commented 4 years ago

Might as well as it should only take a few minutes - I'm doing mine now. Great work Kelli!

aaronmberger-nwfsc commented 4 years ago

Then we can just tell the SRG that the figures will be updated in the final assessment document.

andrew-edwards commented 4 years ago

Nice. I'll do mine in the meeting. Will save the current knitr-cache as something else just in case anything breaks. Thanks Kelli....

kellijohnson-NOAA commented 4 years ago

Sorry it took me so long to do this, next time when I am non-responsive and someone assigns me something feel free to send me an email! I have a short attention span and can sometimes forget what I need to actually be working on.

aaronmberger-nwfsc commented 4 years ago

No worries, the management talk built and I see the fix worked for figures in there. I'm using this naming convention for uploading figures to the SRG meeting google drive: JTC-hake-management

This will get rid of questions about: What is beamer?

aaronmberger-nwfsc commented 4 years ago

One thing that we will still need to consider is that the model files themselves (e.g., report files) will still have the 2x SSB in there. So for example, we need to archive the base model for NWFSC and when we do that we will want to use a MLE and MCMC run that have the correct results in the report files.

andrew-edwards commented 4 years ago

Leaving open - all fixed for talks, just:

kellijohnson-NOAA commented 4 years ago

The Report files have always had 2x the biomass in them b/c we have sex = 1. I think that we label SSB in the report file enough times with "Female Spawning Biomass" that it has always been clear when we report SSB that it pertains to females only and if someone is looking at the Report files they should assume that they are looking at male and female spawning biomass. I am checking off the second box regarding "Aaron's ... archiving" b/c nothing needs to be done to be consistent with previous years.

andrew-edwards commented 4 years ago

Closing, as all fixed and on Friday I checked the document (second tickbox in #700).