std::max does not always propagate NaNs ; this could mess up integrator convergence

[antequ]

Just a heads up that std::max only propagates NaN if it's passed in the first argument. See the second answer here: https://stackoverflow.com/questions/1632145/use-of-min-and-max-functions-in-c In particular, std::max(x, NaN) gives us x for any x.

In our implicit integrators, IntegratorBase::CalcStateChangeNorm() uses this line to aggregate the dq, dv, and dz norms: https://github.com/RobotLocomotion/drake/blob/685afb2ac736228d9eaf7e382b91a20e30f39df6/systems/analysis/integrator_base.h#L1833 return max(z_nrm, max(q_nrm, v_nrm));

This means that if q_nrm or v_nrm is NaN, CalcStateChangeNorm just believes it to be z_nrm. This value is eventually used to calculate dx_norm, which is used in the logic determining integrator convergence. If the integrator uses no error control, it can mistakenly produce a final value containing NaNs instead of retrying with smaller step sizes.

[sherm1]

Oh, that's annoying! And std::fmax(a,b) treats NaNs as "missing data" so that if a is NaN it returns b. It seems that there is no standard NaN-propagating max() in the C++ library :(.

[jwnimmer-tri]

@sherm1 I know there have been a couple PRs recently related to NaNs in the integrators. Is this issue close-able now?

[edrumwri]

@jwnimmer-tri: all of the integrator (and Simulator too) code should be checked before signing off on this issue. Anecdotally, the code seems to be robust to NaNs, but some diligence is still required.