search

NOAA-EDAB / Rpath

R implementation of the mass balance model Ecopath with Ecosim
http://NOAA-EDAB.github.io/Rpath
Other
29 stars 12 forks source link

Different results between old repo and new one #45

Closed okenk closed 1 year ago

okenk commented 2 years ago

I was previously using an Rpath package installation from the old repo, https://github.com/slucey/RpathDev. I recently reinstalled Rpath using the new repo, and a model that was previously balanced is no longer balanced (😱 ). I think it might have to do with how the stanzas are calculated? Has this changed?

okenk commented 2 years ago

I looked into this and think it was some commits about a year ago by @kaydin (maybe 8735a8383d888bced13247dcd60149bcc93d2185). I noticed the commit says it changed the leading stanza to the last stanza. Some of my "leading" stanzas that are informed by more data are the juvenile stanzas, not the adult ones. I would love my model to work again without having to use an outdated version!

okenk commented 2 years ago

I believe this same commit caused issue #37. It would be great to revert to the previous version of the code.

kaydin commented 2 years ago

Those commits were part of a large merge to fix some other underlying problems with the stanza code and reversions would bring back those breakages, so it’s not a simple matter of reversion. Of course we’ll try and solve both issues. Thanks.

On Wed, Jun 29, 2022 at 2:06 AM Kiva Oken @.***> wrote:

I believe this same commit caused issue #37 https://github.com/NOAA-EDAB/Rpath/issues/37. It would be great to revert to the previous version of the code.

— Reply to this email directly, view it on GitHub https://github.com/NOAA-EDAB/Rpath/issues/45#issuecomment-1169570351, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB6LG3DQRVZ2BPKUHVHW6X3VRPRWXANCNFSM52D7N5TQ . You are receiving this because you were mentioned.Message ID: @.***>

kaydin commented 2 years ago

Just to add, one of the previous issues was that the last age class was truncating early in the original version, so it’s possible that in the fixed version, if you’re calculating biomass of an older pool from a younger one that your balance biomass may change. I’m traveling now so I apologize I can’t look further until next week.

On Tue, Jun 28, 2022 at 9:12 PM Kiva Oken @.***> wrote:

I looked into this and think it was some commits about a year ago by @kaydin https://github.com/kaydin (maybe 8735a83 https://github.com/NOAA-EDAB/Rpath/commit/8735a8383d888bced13247dcd60149bcc93d2185). I noticed the commit says it changed the leading stanza to the last stanza. Some of my "leading" stanzas that are informed by more data are the juvenile stanzas, not the adult ones. I would love my model to work again without having to use an outdated version!

— Reply to this email directly, view it on GitHub https://github.com/NOAA-EDAB/Rpath/issues/45#issuecomment-1169431895, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB6LG3DDGGFYGJJ6PWPTWLTVROPILANCNFSM52D7N5TQ . You are receiving this because you were mentioned.Message ID: @.***>

okenk commented 2 years ago

I see, thanks for looking into this! I have an EE that went from 0.90 to 3.94 once the stanza code changed. My intuition is this seems like a very large change just for an age truncation adjustment, though I know sometimes small changes can have big impacts with these complicated models...

kaydin commented 2 years ago

Yeah that definitely sounds like too much change to be explained by the age truncation. If you could send me your model files (to @.***) I can have a look when back in the office next week (I don’t have a good example of a juvenile leading stanza model to test).

On Wed, Jun 29, 2022 at 2:41 PM Kiva Oken @.***> wrote:

I see, thanks for looking into this! I have an EE that went from 0.90 to 3.94 once the stanza code changed. My intuition is this seems like a very large change just for an age truncation adjustment, though I know sometimes small changes can have big impacts with these complicated models...

— Reply to this email directly, view it on GitHub https://github.com/NOAA-EDAB/Rpath/issues/45#issuecomment-1170359257, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB6LG3DPCP5BRXJDAPT5P7LVRSKDTANCNFSM52D7N5TQ . You are receiving this because you were mentioned.Message ID: @.***>

slucey commented 1 year ago

Linked to issue #37

slucey commented 1 year ago

@kaydin temporarily resolved by rolling back stanza code. Need to investigate further but should be working in most cases right now.

kaydin commented 1 year ago

Hi Kiva,

I've only had a brief time to look at this in between traveling (will be out after today until 1-August). Given your time schedule for submission, I've pushed a branch to the repo called 'stanza_rollback' that you can install from github, that simply rolls back that one piece to your preferred version. PLEASE NOTE that this brings back the bug in the older age classes that will still need to be fixed, so the final version will produce different biomasses after I can merge the two fixes in August (but hopefully not very big differences comparatively).

Best, Kerim

slucey commented 1 year ago

@kaydin we need to merge stanza_rollback into dev

kaydin commented 1 year ago

@slucey understood, but that would break any EBS examples and the fitting branch, as the rollback version is broken for the EBS and our fitting tests - high priority to resolve in a way that works for both.

slucey commented 1 year ago

@kaydin ok. Let's get @rklasky on this issue and mark as our highest priority. Maybe we can set up a meeting early next week to get him going.

slucey commented 1 year ago

@okenk we're trying to finally get this resolved. Do you have a reproducible example we could use for testing? Feel free to email me.

okenk commented 1 year ago

Model, diet, stanzas

I think the issue is some of the leading stanzas are the juvenile group. It is an estuary model and the state sampling program more effectively captures juveniles of some species.

slucey commented 1 year ago

@okenk that makes a lot of sense...thanks for sharing. Hopefully we'll have this fully resolved soon.

slucey commented 1 year ago

@okenk I'm missing a few parameters to build your model. What did you use for your unassimilated biomass values? Also, I need the first and last months for each stanza group.

okenk commented 1 year ago

I believe the unassimilated biomass was 0. The R script that runs the model is here-- hopefully that helps!

okenk commented 1 year ago

Unassimilated biomass was not zero!

slucey commented 1 year ago

@okenk got it thanks! Not sure how I missed the first/last columns before.

slucey commented 1 year ago

Closed by pull request #70