histAssess loop just overwrites

[mle2718]

Hi Folks, I think there's a bug here that only affects model runs when there are multiple stocks going at the same time. This bit of code loops over stocks and makes an object called assess_vals

https://github.com/lkerr/groundfish-MSE/blob/170517f04d2ae9fe75ee35beb6f79267fabd6430/processes/runSim.R#L65-L70

But, since nothing is done inside the loop, we just keep overwriting when nstock>1.

This doesn't affect me right now, since I am running things with histAssess ==FALSE

I have not looked into the downstream affects of this beyond the fact that it is used in the https://github.com/lkerr/groundfish-MSE/blob/170517f04d2ae9fe75ee35beb6f79267fabd6430/processes/runSim.R#L74-L76

so my first inclination is to suggest

   for(y in fyear:nyear){
      for(i in 1:nstock){
    if (histAssess == TRUE) {

      assess_vals <- get_HistAssess(stock = stock[[i]])

    }

        stock[[i]] <- get_J1Updates(stock = stock[[i]])
      }

But I am not confident in this, because I don't really understand what's going on with assess_vals.

[samtruesdell]

Thanks for the flag Min-Yang this looks like a bug to me too. A quick search for assess_vals brings up a few instances. Another possibility is to make assess_vals a list of length nstock.

I'm not able to look into this any further at the moment

Sam

[mle2718]

@samtruesdell thanks for the confirmation. I've recharacterized it. I don't think it substantively affects anything. Yet.

[mle2718]

@jerellejesse and @lkerr : I think this is going to be hardest bug to fix before getting the economic model to work properly. As currently coded, this loop means that assessvals picks up the 'last' stock's assess_vals when I try to run multiple stocks at the same time.

Here is the result of my find/grep on assess_vals:

./functions/assessment/get_AssessVals.R:replacement <- assess_vals$assessdat[assess_vals$assessdat$MSEyr == y,]
./functions/popdy/get_AssessVals.R:replacement <- assess_vals$assessdat[assess_vals$assessdat$MSEyr == y,]
./functions/popdy/get_J1Updates.R:        if(y %in% assess_vals$assessdat$MSEyr){
./functions/popdy/get_recruits.R:            assess_vals <- get_HistAssess(stock = stock[[i]])
./functions/popdy/get_recruits.R:            pred<-remp(1,tail(as.numeric(assess_vals$assessdat$R,20)))
./functions/popdy/get_recruits.R:            assess_vals <- get_HistAssess(stock = stock[[i]])
./functions/popdy/get_recruits.R:              pred <- cR * remp(1, tail(as.numeric(assess_vals$assessdat$R), Rnyr))
./functions/popdy/get_recruits.R:              pred <-  cR * (SSB/SSBhinge) * remp(1, tail(as.numeric(assess_val$assessdat$R), Rnyr))
./processes/runSim.R:      assess_vals <- get_HistAssess(stock = stock[[i]])

Because assess_vals is used in multiple places, I don't think my solution in the initial post is a good approach.

Perhaps assess_vals becomes an element in the list of stock (but it's not necessary to save this as an output) -- then things can be done within.

[jerellejesse]

Okay, I will prioritize this and can discuss with Lisa if I need more help.

[jerellejesse]

I just wanted to give an update that this is almost done. There were some issues that arose while fixing it. Right now I am getting an error when moving to a second rep. I'll keep this issue updated when I figure it out!

[mle2718]

I just wanted to give an update that this is almost done. There were some issues that arose while fixing it. Right now I am getting an error when moving to a second rep. I'll keep this issue updated when I figure it out!

@jerellejesse thanks for the update. I'm completely swamped with other projects, so it's not a huge deal. With a little luck I'll have some time to turn back to this towards the end of Jan. Let me know if I can help in any way --although your R skills are likely much better than mine.

[jerellejesse]

Sorry @mle2718 I forgot to let you know that I finished fixing this in JJ-histAssess branch. The histAssess loop should now be part of the stock object and I tracked down the indexing issues. I think I will merge my changes into the dev branch if that sounds good to @lkerr and we can test.

[mle2718]

@jerellejesse it's been a while since I worked on this. I will try to pick it up at the end of Feb. The last thing I did was merge EconOnly2<-Dev in october. Can you @ me when the JJ-histAssess is in Dev?

[mle2718]

@jerellejesse. Looks like PR #267 merges JJ-histAssess into Dev.

I'm going to mark this as closed.

I'm going to merge Dev into my EconOnly2 branch to pick up your changes.