PIFSCstockassessments / ss3diags

R package with advanced diagnostics to evaluate a Stock Synthesis model. Diagnostics include residual analyses, hindcasting and cross-validation techniques, and retrospective analyses.
http://pifscstockassessments.github.io/ss3diags/
2 stars 4 forks source link

SSdeltaMVLN omits first model year #34

Closed N-DucharmeBarth-NOAA closed 2 years ago

N-DucharmeBarth-NOAA commented 2 years ago

Hello, It appears that the SSdeltaMVLN omits the first model year from ensuing calculations. Currently line 39 calculates allyrs the following way:

allyrs <- unique(as.numeric(gsub(paste0(status[1], "_"), "", hat$Label[grep(paste0(status[1], "_"), hat$Label)])))[-1]

Perhaps it should omit the [-1] at the end of the line unless this is intentional?

MOshima-PIFSC commented 2 years ago

@Henning-Winker @jabbamodel, I wanted to check with you and see if there was a specific reason for ignoring the first year of this vector. When I checked this line using the simple example model we have, it removed the first two years; the model start year is 26 but the allyrs vector starts at 28 because the first year of Bratio reported is 27. If this is meant to be this way, I suggest renaming the variable allyrs to reduce confusion for users reading through the code and we can make a note in the documentation as to why it ignores the first year (or two). Otherwise, this would be a quick fix. NOTE: if the vector should include the start year, it should use the years from "F" because F is estimated for the first year. The code would then be:

allyrs <- unique(as.numeric(gsub(paste0(status[2], "_"), "", hat$Label[grep(paste0(status[2], "_"), hat$Label)])))
MOshima-PIFSC commented 2 years ago

After clarification from Henning, the current code is correct. Due to the way that SS3 thins the covariance matrix, there are missing years but at least every 3 years is reported. Bratio is first reported for year 2, so adding the [-1] at the end of the search gets the third year. I suggest closing this issue and opening another to add this information to the documentation of the function. I also suggest changing the name of the variable from allyrs to something more general to avoid future confusion.