Skip to content

Commit

Permalink
nmparser: guard against NaN for all incoming floats
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
kyleam committed Oct 17, 2024
1 parent 1fe82d5 commit 275135e
Show file tree
Hide file tree
Showing 11 changed files with 42 additions and 33 deletions.
2 changes: 1 addition & 1 deletion integration/nmless/bbi_summary_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ func TestSummaryHappyPath(tt *testing.T) {
"example2_bayes", // Bayes (5 est methods, from NONMEM examples)
"iovmm", // Mixture model. Also has parameter_near_boundary and final_zero_gradient heuristics.
"acop-iov", // fake model with 62 OMEGAS (fake iov)
"acop-nan", // NaN values for objective function
"acop-nan", // NaN values in several spots
"onlysim", // $SIM with ONLYSIM.
"chain", // CHAIN method that lacks estimation data.
"12-empty-problem", // $PROBLEM is an empty string
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,12 +49,12 @@
"theta": [
2.31034,
54.9596,
464.659,
-999999999,
-0.0805722,
4.1303
],
"omega": [
0.0964407,
-999999999,
0,
0.153571
],
Expand All @@ -65,14 +65,14 @@
"std_err": {
"theta": [
0.086147,
3.32914,
-999999999,
29.6177,
0.0555149,
1.35989
],
"omega": [
0.0200146,
10000000000,
-999999999,
0.026733
],
"sigma": [
Expand Down Expand Up @@ -105,7 +105,7 @@
0,
0,
0,
0
-999999999
],
"omega": [
0,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,14 @@ No Heuristic Problems Detected
| THETA | NAME | ESTIMATE | STDERR (RSE) |
+-------+--------+------------+-------------------+
| TH 1 | THETA1 | 2.31034 | 0.086147 (3.7%) |
| TH 2 | THETA2 | 54.9596 | 3.32914 (6.1%) |
| TH 3 | THETA3 | 464.659 | 29.6177 (6.4%) |
| TH 2 | THETA2 | 54.9596 | - |
| TH 3 | THETA3 | -999999999 | 29.6177 (0.0%) |
| TH 4 | THETA4 | -0.0805722 | 0.0555149 (68.9%) |
| TH 5 | THETA5 | 4.1303 | 1.35989 (32.9%) |
+-------+--------+------------+-------------------+
+------------+------+----------+---------------+
| OMEGA | ETA | ESTIMATE | SHRINKAGE (%) |
+------------+------+----------+---------------+
| OMEGA(1,1) | ETA1 | 0.096441 | 17.511500 |
| OMEGA(2,2) | ETA2 | 0.153571 | 2.059620 |
+------------+------+----------+---------------+
+------------+------+-------------------+---------------+
| OMEGA | ETA | ESTIMATE | SHRINKAGE (%) |
+------------+------+-------------------+---------------+
| OMEGA(1,1) | ETA1 | -999999999.000000 | 17.511500 |
| OMEGA(2,2) | ETA2 | 0.153571 | 2.059620 |
+------------+------+-------------------+---------------+
14 changes: 7 additions & 7 deletions integration/nmless/testdata/bbi_summary/acop-nan/acop-nan.ext
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,10 @@ TABLE NO. 1: First Order Conditional Estimation with Interaction: Goal Funct
20 2.30528E+00 5.58065E+01 4.72255E+02 -8.05144E-02 4.12591E+00 1.00000E+00 9.09862E-02 0.00000E+00 2.06974E-01 2638.5029836424560
25 2.31302E+00 5.44961E+01 4.65370E+02 -8.07920E-02 4.13612E+00 1.00000E+00 9.61335E-02 0.00000E+00 1.57375E-01 2636.8778706340345
28 2.31034E+00 5.49596E+01 4.64659E+02 -8.05722E-02 4.13030E+00 1.00000E+00 9.64407E-02 0.00000E+00 1.53571E-01 2636.8457703290751
-1000000000 2.31034E+00 5.49596E+01 4.64659E+02 -8.05722E-02 4.13030E+00 1.00000E+00 9.64407E-02 0.00000E+00 1.53571E-01 2636.8457703290751
-1000000001 8.61470E-02 3.32914E+00 2.96177E+01 5.55149E-02 1.35989E+00 1.00000E+10 2.00146E-02 1.00000E+10 2.67330E-02 0.0000000000000000
-1000000004 0.00000E+00 0.00000E+00 0.00000E+00 0.00000E+00 0.00000E+00 1.00000E+00 3.10549E-01 0.00000E+00 3.91882E-01 0.0000000000000000
-1000000005 0.00000E+00 0.00000E+00 0.00000E+00 0.00000E+00 0.00000E+00 1.00000E+10 3.22245E-02 1.00000E+10 3.41085E-02 0.0000000000000000
-1000000006 0.00000E+00 0.00000E+00 0.00000E+00 0.00000E+00 0.00000E+00 1.00000E+00 0.00000E+00 1.00000E+00 0.00000E+00 0.0000000000000000
-1000000007 0.00000E+00 3.70000E+01 0.00000E+00 0.00000E+00 0.00000E+00 0.00000E+00 0.00000E+00 0.00000E+00 0.00000E+00 0.0000000000000000
-1000000008 -3.39427E-03 1.00007E-04 -7.27007E-04 -1.46088E-01 2.43350E-03 0.00000E+00 4.84108E-02 0.00000E+00 -5.58021E-03 0.0000000000000000
-1000000000 2.31034E+00 5.49596E+01 NaN -8.05722E-02 4.13030E+00 1.00000E+00 NaN 0.00000E+00 1.53571E-01 2636.8457703290751
-1000000001 8.61470E-02 NaN 2.96177E+01 5.55149E-02 1.35989E+00 1.00000E+10 2.00146E-02 NaN 2.67330E-02 0.0000000000000000
-1000000004 NaN 0.00000E+00 0.00000E+00 0.00000E+00 0.00000E+00 1.00000E+00 3.10549E-01 0.00000E+00 3.91882E-01 0.0000000000000000
-1000000005 0.00000E+00 0.00000E+00 0.00000E+00 NaN 0.00000E+00 1.00000E+10 3.22245E-02 1.00000E+10 3.41085E-02 0.0000000000000000
-1000000006 0.00000E+00 0.00000E+00 0.00000E+00 0.00000E+00 NaN 1.00000E+00 0.00000E+00 1.00000E+00 0.00000E+00 0.0000000000000000
-1000000007 0.00000E+00 3.70000E+01 0.00000E+00 0.00000E+00 0.00000E+00 NaN 0.00000E+00 0.00000E+00 0.00000E+00 0.0000000000000000
-1000000008 -3.39427E-03 1.00007E-04 -7.27007E-04 -1.46088E-01 2.43350E-03 0.00000E+00 4.84108E-02 0.00000E+00 NaN 0.0000000000000000
3 changes: 1 addition & 2 deletions parsers/nmparser/get_model_output.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import (
"errors"
"fmt"
"path/filepath"
"strconv"
"strings"

"github.com/metrumresearchgroup/bbi/utils"
Expand Down Expand Up @@ -90,7 +89,7 @@ func GetModelOutput(lstPath string, ext ModelOutputFile, grd bool, shk bool) (Su
log.Trace("error reading cpu file: ", err)
} else {
results.RunDetails.OutputFilesUsed = append(results.RunDetails.OutputFilesUsed, filepath.Base(cpuFilePath))
cpuTime, err := strconv.ParseFloat(strings.TrimSpace(cpuLines[0]), 64)
cpuTime, err := parseFloatReplaceNaN(strings.TrimSpace(cpuLines[0]))
if err != nil {
// this is set to trace as don't want it to log normally as could screw up json output that
log.Trace("error parsing cpu time: ", err)
Expand Down
4 changes: 2 additions & 2 deletions parsers/nmparser/parse_lst_file.go
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ func getMatrixData(lines []string, start int) MatrixData {
// populate matrix
for j, row := range rows {
for k, cell := range strings.Fields(row) {
value, err := strconv.ParseFloat(cell, 64)
value, err := parseFloatReplaceNaN(cell)
if err == nil {
matrix[j][k] = value
} else {
Expand Down Expand Up @@ -418,7 +418,7 @@ func mustCalculateConditionNumber(lines []string, start int) float64 {

eigenstarted = true
for _, s := range strings.Fields(line) {
eigenvalue, err := strconv.ParseFloat(s, 64)
eigenvalue, err := parseFloatReplaceNaN(s)
if err != nil {
panic(fmt.Sprintf("Attempting to calculate condition number but could not parse eigenvalues -- %v", err))
}
Expand Down
4 changes: 2 additions & 2 deletions parsers/nmparser/parse_run_details.go
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ func ParseRunDetails(lines []string) RunDetails {
case strings.Contains(line, "NO. OF FUNCTION EVALUATIONS USED"):
runDetails.FunctionEvaluations, _ = strconv.ParseInt(replaceTrim(line, "NO. OF FUNCTION EVALUATIONS USED:"), 10, 64)
case strings.Contains(line, "NO. OF SIG. DIGITS IN FINAL EST.:"):
runDetails.SignificantDigits, _ = strconv.ParseFloat(replaceTrim(line, "NO. OF SIG. DIGITS IN FINAL EST.:"), 64)
runDetails.SignificantDigits = strToFloat(replaceTrim(line, "NO. OF SIG. DIGITS IN FINAL EST.:"))
case strings.Contains(line, "Elapsed estimation"):
runDetails.EstimationTime = append(runDetails.EstimationTime,
parseFinalTime(line))
Expand All @@ -68,7 +68,7 @@ func ParseRunDetails(lines []string) RunDetails {
case strings.Contains(line, "Elapsed postprocess"):
runDetails.PostprocessTime = parseFinalTime(line)
case strings.Contains(line, " #CPUT: Total CPU Time in Seconds,"):
runDetails.CpuTime, _ = strconv.ParseFloat(replaceTrim(line, " #CPUT: Total CPU Time in Seconds,"), 64)
runDetails.CpuTime = strToFloat(replaceTrim(line, " #CPUT: Total CPU Time in Seconds,"))
case strings.Contains(line, "Started"):
runDetails.RunStart = replaceTrim(line, "Started")
case strings.HasPrefix(lineTrimmed, "Finished "):
Expand Down
3 changes: 1 addition & 2 deletions parsers/nmparser/read_cov.go
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package parser

import (
"strconv"
"strings"
)

Expand Down Expand Up @@ -51,7 +50,7 @@ func getThetaMatrix(lines []string) [][]float64 {
thetas := make([]float64, len)

for n := 1; n <= len; n++ {
if f, err := strconv.ParseFloat(fields[n], 64); err == nil {
if f, err := parseFloatReplaceNaN(fields[n]); err == nil {
thetas[n-1] = f
} else {
panic("error converting value in cov file to number: " + fields[n])
Expand Down
4 changes: 2 additions & 2 deletions parsers/nmparser/read_ext.go
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ func ParseParamsExt(ed ExtData) ([]ParametersData, ParameterNames) {
// this is the OBJ which we've gotten elsewhere
continue
}
if n, err := strconv.ParseFloat(val, 64); err == nil {
if n, err := parseFloatReplaceNaN(val); err == nil {
result[i-1] = n
} else {
log.Println("Did you use a non-default format parameter? bbi does not support non-default format parameters.")
Expand Down Expand Up @@ -163,7 +163,7 @@ func ParseConditionNumberExt(ed ExtData) []ConditionNumDetails {
fields := strings.Fields(line)
step, _ := strconv.Atoi(fields[0])
if step == -1000000003 {
condNum, _ = strconv.ParseFloat(fields[1], 64)
condNum = strToFloat(fields[1])

break
} else {
Expand Down
3 changes: 1 addition & 2 deletions parsers/nmparser/read_grd.go
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package parser

import (
"strconv"
"strings"
)

Expand Down Expand Up @@ -64,7 +63,7 @@ func ParseGrdData(ed ExtData) ([]ParametersData, ParameterNames) {
if i == 0 {
continue
}
if n, err := strconv.ParseFloat(val, 64); err == nil {
if n, err := parseFloatReplaceNaN(val); err == nil {
result[i-1] = n
} else {
panic("error converting value in grd file to number: " + val)
Expand Down
12 changes: 12 additions & 0 deletions parsers/nmparser/utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -73,3 +73,15 @@ func strToFloat(s string) float64 {

return f
}

// parseFloatReplaceNaN converts s to a float64. If the result is NaN,
// DefaultFloat64 is returned as the value. Any errors are relayed as is from
// strconv.ParseFloat.
func parseFloatReplaceNaN(s string) (float64, error) {
f, err := strconv.ParseFloat(s, 64)
if math.IsNaN(f) {
f = DefaultFloat64
}

return f, err
}

0 comments on commit 275135e

Please sign in to comment.