Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

nmparser: guard against NaN for all incoming floats #334

Merged
merged 2 commits into from
Oct 22, 2024
Merged

nmparser: guard against NaN for all incoming floats #334

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1fe82d5
nmparser: add docstring for strToFloat
kyleam Oct 8, 2024
275135e
nmparser: guard against NaN for all incoming floats
kyleam Oct 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion integration/nmless/bbi_summary_test.go
View file
Open in desktop
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
10 changes: 5 additions & 5 deletions integration/nmless/testdata/bbi_summary/aa_golden_files/acop-nan.golden.json
View file
Open in desktop
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,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

@kyleam kyleam Oct 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense. Thanks for the explanation.

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
16 changes: 8 additions & 8 deletions integration/nmless/testdata/bbi_summary/aa_golden_files/acop-nan.golden.txt
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
14 changes: 14 additions & 0 deletions parsers/nmparser/utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -71,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
}