From 1fe82d566c1f95b147430e48ec23afc716ca208d Mon Sep 17 00:00:00 2001 From: Kyle Meyer Date: Tue, 8 Oct 2024 16:01:27 -0400 Subject: [PATCH 1/2] nmparser: add docstring for strToFloat --- parsers/nmparser/utils.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/parsers/nmparser/utils.go b/parsers/nmparser/utils.go index f6d97845..57418404 100644 --- a/parsers/nmparser/utils.go +++ b/parsers/nmparser/utils.go @@ -63,6 +63,8 @@ func CheckIfBayesian(results SummaryOutput) bool { return isBayesian } +// strToFloat converts s to a float64. If the result is NaN or any error is +// encountered, DefaultFloat64 is returned as the value. func strToFloat(s string) float64 { f, err := strconv.ParseFloat(s, 64) if err != nil || math.IsNaN(f) { From 275135ed130f28c5de19813845a2c818649a66f2 Mon Sep 17 00:00:00 2001 From: Kyle Meyer Date: Tue, 8 Oct 2024 16:01:27 -0400 Subject: [PATCH 2/2] nmparser: guard against NaN for all incoming floats NONMEM output may contain NaN values for floats. Since fc3af5b6 (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. --- integration/nmless/bbi_summary_test.go | 2 +- .../aa_golden_files/acop-nan.golden.json | 10 +++++----- .../aa_golden_files/acop-nan.golden.txt | 16 ++++++++-------- .../testdata/bbi_summary/acop-nan/acop-nan.ext | 14 +++++++------- parsers/nmparser/get_model_output.go | 3 +-- parsers/nmparser/parse_lst_file.go | 4 ++-- parsers/nmparser/parse_run_details.go | 4 ++-- parsers/nmparser/read_cov.go | 3 +-- parsers/nmparser/read_ext.go | 4 ++-- parsers/nmparser/read_grd.go | 3 +-- parsers/nmparser/utils.go | 12 ++++++++++++ 11 files changed, 42 insertions(+), 33 deletions(-) diff --git a/integration/nmless/bbi_summary_test.go b/integration/nmless/bbi_summary_test.go index 5dd5ef40..dc1776db 100644 --- a/integration/nmless/bbi_summary_test.go +++ b/integration/nmless/bbi_summary_test.go @@ -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 diff --git a/integration/nmless/testdata/bbi_summary/aa_golden_files/acop-nan.golden.json b/integration/nmless/testdata/bbi_summary/aa_golden_files/acop-nan.golden.json index 276dc67e..511abd2b 100644 --- a/integration/nmless/testdata/bbi_summary/aa_golden_files/acop-nan.golden.json +++ b/integration/nmless/testdata/bbi_summary/aa_golden_files/acop-nan.golden.json @@ -49,12 +49,12 @@ "theta": [ 2.31034, 54.9596, - 464.659, + -999999999, -0.0805722, 4.1303 ], "omega": [ - 0.0964407, + -999999999, 0, 0.153571 ], @@ -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": [ @@ -105,7 +105,7 @@ 0, 0, 0, - 0 + -999999999 ], "omega": [ 0, diff --git a/integration/nmless/testdata/bbi_summary/aa_golden_files/acop-nan.golden.txt b/integration/nmless/testdata/bbi_summary/aa_golden_files/acop-nan.golden.txt index 6556446e..a392aeb9 100644 --- a/integration/nmless/testdata/bbi_summary/aa_golden_files/acop-nan.golden.txt +++ b/integration/nmless/testdata/bbi_summary/aa_golden_files/acop-nan.golden.txt @@ -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 | ++------------+------+-------------------+---------------+ diff --git a/integration/nmless/testdata/bbi_summary/acop-nan/acop-nan.ext b/integration/nmless/testdata/bbi_summary/acop-nan/acop-nan.ext index 587c6d7d..5b56fb56 100644 --- a/integration/nmless/testdata/bbi_summary/acop-nan/acop-nan.ext +++ b/integration/nmless/testdata/bbi_summary/acop-nan/acop-nan.ext @@ -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 diff --git a/parsers/nmparser/get_model_output.go b/parsers/nmparser/get_model_output.go index 65259a88..5b934972 100644 --- a/parsers/nmparser/get_model_output.go +++ b/parsers/nmparser/get_model_output.go @@ -4,7 +4,6 @@ import ( "errors" "fmt" "path/filepath" - "strconv" "strings" "github.com/metrumresearchgroup/bbi/utils" @@ -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) diff --git a/parsers/nmparser/parse_lst_file.go b/parsers/nmparser/parse_lst_file.go index 3cae6b70..e646876b 100644 --- a/parsers/nmparser/parse_lst_file.go +++ b/parsers/nmparser/parse_lst_file.go @@ -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 { @@ -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)) } diff --git a/parsers/nmparser/parse_run_details.go b/parsers/nmparser/parse_run_details.go index 288a82e5..1420ea2f 100644 --- a/parsers/nmparser/parse_run_details.go +++ b/parsers/nmparser/parse_run_details.go @@ -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)) @@ -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 "): diff --git a/parsers/nmparser/read_cov.go b/parsers/nmparser/read_cov.go index 454608de..350fd897 100644 --- a/parsers/nmparser/read_cov.go +++ b/parsers/nmparser/read_cov.go @@ -1,7 +1,6 @@ package parser import ( - "strconv" "strings" ) @@ -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]) diff --git a/parsers/nmparser/read_ext.go b/parsers/nmparser/read_ext.go index 310a1e28..f3bd40ce 100644 --- a/parsers/nmparser/read_ext.go +++ b/parsers/nmparser/read_ext.go @@ -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.") @@ -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 { diff --git a/parsers/nmparser/read_grd.go b/parsers/nmparser/read_grd.go index db8174aa..8afebdf0 100644 --- a/parsers/nmparser/read_grd.go +++ b/parsers/nmparser/read_grd.go @@ -1,7 +1,6 @@ package parser import ( - "strconv" "strings" ) @@ -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) diff --git a/parsers/nmparser/utils.go b/parsers/nmparser/utils.go index 57418404..70a2f148 100644 --- a/parsers/nmparser/utils.go +++ b/parsers/nmparser/utils.go @@ -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 +}