nmparser: guard against NaN for all incoming floats #334
Conversation
| -0.0805722, | ||
| 4.1303 | ||
| ], | ||
| "omega": [ | ||
| 0.0964407, | ||
| -999999999, |
There was a problem hiding this comment.
I must be missing something, but I'm unclear why these golden files are changing to now included NaN's (or, more specifically -999 aka DefaultFloat64). I understand the motivation, but I don't see where we change any test files to now expect NaN in these spots. Thanks in advance for explaining.
There was a problem hiding this comment.
Sorry, I should have explained that somewhere. I tweaked the integration/nmless/testdata/bbi_summary/acop-nan/acop-nan.ext input to introduce NaNs in a bunch of spots. That was an existing test case related to NaNs, but only set the objective function values to NaN in the lst file (75d8f5c, 2022-06-23).
So the golden file changes are coming from me adjusting the input.
There was a problem hiding this comment.
I've updated the commit message to include this:
For testing, piggyback on top of summary's existing acop-nan test case by setting many values in its ext file to NaN.
There was a problem hiding this comment.
That makes sense. Thanks for the explanation.
NONMEM output may contain NaN values for floats. Since fc3af5b (nmparser: map NaN's to DefaultFloat64 too, 2022-06-23), the nmparser code has guarded against NaNs in spots that use the strToFloat helper. However, that doesn't cover the places that invoke strconv.ParseFloat directly. That's a problem because the summary command will error when trying to encode an NaN to JSON. Adjust all the remaining strconv.ParseFloat calls to guard against NaN. Simply using strToFloat in all these spots isn't a good option because that silently replaces errors with DefaultFloat64. Add a new helper that maps NaN to DefaultFloat64 but, unlike strToFloat, relays any errors. If a spot does something with the error [*], use the new helper. On the other hand, if a spot ignores the error, use strToFloat. For testing, piggyback on top of summary's existing acop-nan test case by setting many values in its ext file to NaN. Thanks to @callistosp for reporting the original failure case. [*] Unfortunately the spots that process the error panic, while ideally they would bubble up the error as a return value. Still, using strToFloat is a step in the wrong direction.
kyleam
force-pushed
the
summary-more-nan
branch
from
9c37b09 to
275135e
Compare
@callistosp reported a case where
bbi nonmem summary --jsonfails to encode the result:To avoid this and related errors, this PR makes all spots that parse floats replace
NaNwithDefaultFloat64.