Possible Undefined Behavior / NaNs

[juliangaal]

I think this code produces NaNs or undefined behavior, if the time difference is 0.

https://github.com/vectr-ucla/direct_lidar_inertial_odometry/blob/a524000eb2e18a801a9222410650ddb12eddde39/src/dlio/odom.cc#L952-L954

From the reference:

If the second operand is zero, the behavior is undefined, except that if floating-point division is taking place and the type supports IEEE floating-point arithmetic (see std::numeric_limits::is_iec559), then:

• if one operand is NaN, the result is NaN
• dividing a non-zero number by ±0.0 gives the correctly-signed infinity and FE_DIVBYZERO is raised
• dividing 0.0 by 0.0 gives NaN and FE_INVALID is raised

The straightforward solution would be to check for 0 before adjusting imu_rates. I am not sure if that would break some other parts of the code that depend on imu_rates.

I can create a PR and test with the provided data if you agree this could be an issue.

[kennyjchen]

Ah, good catch! Yes a PR would be great. Thanks so much @juliangaal :)

[juliangaal]

Sounds good. I'll take care of it in the next couple of days

[kennyjchen]

@juliangaal -- Thanks again for the catch. I added a quick fix for when, for some reason, dt == 0. Shouldn't happen unless there is a data hiccup.

[juliangaal]

I'm surprised you're not simply returning. Could you explain your thinking? To my understanding, it shouldn't affect the calculation of the average rate because it just does an std::accumulate on the vector and doesn't care about the number of elements.

[kennyjchen]

It's not a guarantee that, just because dt == 0 then the previous measurement and the current measurement are the same. It could be possible that the measurements are different and distinct, but for whatever reason the timestamps are equal. Yes returning won't affect the average rate calculation, but that's purely for visualization. If we return when dt == 0, that would throw away that measurement.

I suppose alternatively, if dt==0, that could be an indicator that a measurement was duplicated during data passing, but I would rather risk duplicating a measurement than losing information. In any case, in practice this should rarely happen, so either way would handle it fine. Just my thinking.

[juliangaal]

great, thanks.