fix: Fix SAVE gotcha with read_snow17_parameters#55
Conversation
Fixing bug in read_snow17_parameters subroutine due to local variable being initialized at declaration, leading to values carried over from previous invocations of the subroutine, and from that causing the loading of the SECOND catchment's config param file to fail to load and cause the execution to be stopped.
|
Great fix, Robert. I think this was discussed pretty well in #44 -- maybe your fix can close this issue? (and definitely refer to it). |
|
btw great to see snow17 getting some attention. we do want to have it as a standard/base option in nextgen. |
|
Hmm ... yes, this addresses the same bug described in #44. However, I see there is also #47, which would also address #44 and then do a little bit more. Given this bug is breaking the module's compatibility with NextGen, it may make more sense to merge this PR (a more minimal solution) first and return to #47 at some point later. But regardless, one of them needs to be fully approved and merged to fix the issue. |
|
Also, FWIW, there is a very similar issue and fix in Sac-SMA, in PR NOAA-OWP/sac-sma#16. |
|
Definitely ... the two codes came from the same source, which was run in a different use case, and inherited some inconsistencies with the nextgen driver. I'm glad this is getting resolved now. |
|
I don't have time to fully review it now (as in run test it myself), but I can at least concur that these changes are needed. |
3 participants
Fixing bug in
read_snow17_parameterssubroutine. The overall result of this bug is that it is not currently possible to use this with ngen whenever more than one catchment formulation uses this as a BMI module.Bug Description
For the
read_snow17_parameterssubroutine, the localiosvariable is used by eachreadto get the next line of the given parameter file. A test ofiosis used to determine when all lines of the parameter file have been read.The problem is that the value of
iosis being initialized as part of its declaration. As noted here and elsewhere, initializing at declaration implies theSAVEattribute is applied to the variable. The effect is thatiosretains its value between calls.Thus,
iosis only properly set the first time the subroutine is run. In the second run, the test of whether the file has been completely read yields a false positive immediately, before any lines and parameter values have been read. That, in turn, leads to an error condition and astop.Changes
The declaration of
iosis changed to not include initialization, and then a subsequent assignment statement is used to properly initialize its expected value for the start ofread_snow17_parameters.Testing
Local testing has been performed to ensure the
read_snow17_parametersnow completes without stopping execution.