campreilly / UnderSeaModelingLibrary

The Under Sea Modeling Library (USML) is a collection of C++ software development modules for sonar modeling and simulation.
Other
42 stars 22 forks source link

`refraction_catenary` error in some clang-17 build configurations #271

Closed cconvey closed 1 day ago

cconvey commented 3 days ago

Some builds of USML consistently cause usml_test to report this error:

...
=== refraction_test: refraction_catenary ===
time step = 0.1 secs freq = 10000 Hz
writing tables to /home/cconvey/r/cconvey/usml/profiling/waveq3d/test/refraction_catenary.csv
/home/cconvey/r/cconvey/usml/profiling/waveq3d/test/refraction_test.cc(773): error: in "waveq3d_refraction_test/refraction_catenary": absolute value of d_alt{nan} exceeds 2
wave propagates for 66.8 secs
max error = 1.90482 meters
=== refraction_test: refraction_munk_range ===
...

Build configurations that always trigger this error:

Build configurations that never trigger this error:

(These are the only build configurations I checked.)

cconvey commented 3 days ago

When debugging this, it might help to enable signalling-NaNs in the floating-point environment.

cconvey commented 2 days ago

I've narrowed the (proximate?) root cause to this line: https://github.com/campreilly/UnderSeaModelingLibrary/blob/d4e140920e2be5cbd068c60c2a8e68bcf082891d/waveq3d/test/refraction_test.cc#L748

When s takes on a value slightly below 1.0 (0.99999999999999988898), then in the following line: https://github.com/campreilly/UnderSeaModelingLibrary/blob/d4e140920e2be5cbd068c60c2a8e68bcf082891d/waveq3d/test/refraction_test.cc#L749 acosh(sqrt(s)) returns NaN, which then propagates through the rest of the math.

The error goes away when I clamp the lower-bound of s:

double s = max<double>(1.0, -0.5 * ((sinT2 * cos(t) - 1.0) / cosT2 - 1.0);

@campreilly : I'm not really qualified to decide if this is an appropriate fix, so I'll leave the rest up to you.

campreilly commented 2 days ago

This is an appropriate fix. You can make the fix if you'd like. Otherwise, I'll handle it in the next update.

cconvey commented 2 days ago

Sounds good. I'll try to have a PR in the next few days, but it's cool if you beat me to it.