rust-num / num-traits

Numeric traits for generic mathematics in Rust
Apache License 2.0
694 stars 131 forks source link

Add unit tests for trait method `float::Float::integer_decode` #327

Open mtilda opened 2 months ago

mtilda commented 2 months ago

Motivation

While working on #326, I saw an opportunity to add unit test coverage for the trait method float::Float::integer_decode. I hope others find this helpful when troubleshooting this method.

Changes

mtilda commented 2 months ago

@cuviper this is ready for review.

I'm especially interested to hear your thoughts about whether NaN values should be recoverable using the formula sign mantissa 2^exponent.

cuviper commented 2 months ago

I'm especially interested to hear your thoughts about whether NaN values should be recoverable using the formula sign mantissa 2^exponent.

It's a good observation -- no, I don't think there's any way to recover a NaN, apart from recognizing the special exponent and taking a different branch. Even then it will be hard to match the exact NaN bits, signaling or not, etc.

Subnormals are also problematic, but those might be recoverable if you're more careful about exp2 underflow. Something like this, but I haven't tested it:

if exponent >= MIN_EXP {
    sign * magnitude * exponent.exp2()
} else {
    // MIN_POSITIVE == 2^(MIN_EXP - 1)
    (sign * magnitude * MIN_POSITIVE) * (exponent - (MIN_EXP - 1)).exp2()
}

I'm ignoring type conversions there, and we don't have generic versions of those constants either. Even then, some hardware will flush subnormal arithmetic to zero anyway, so it still may not work.

So I think it's probably not worth the effort to document anything more complicated, and we can just say that the simple formula only works for zero/normal/infinite values (per classify()).

mtilda commented 2 months ago

Thanks for the feedback!


we can just say that the simple formula only works for zero/normal/infinite values (per classify()).

I just pushed a change to update the doc comment.


By the way, I just learned about Rust's documentation testing feature 😅 I'm new to Rust, so thanks for being patient.

Should I move the tests I wrote to the method doc strings, or keep them in the tests module?

mtilda commented 2 months ago

I just added test cases for subnormals. The recovery code seems to work (within tolerance) for those, but I think they are being treated as zero.

cuviper commented 3 weeks ago

By the way, I just learned about Rust's documentation testing feature 😅 I'm new to Rust, so thanks for being patient.

Should I move the tests I wrote to the method doc strings, or keep them in the tests module?

Doc tests are nice, but we don't want to overwhelm the reader. I think yours are fine as unit tests.

I just added test cases for subnormals. The recovery code seems to work (within tolerance) for those, but I think they are being treated as zero.

Yes, your 1e-6 tolerance is much greater than the value in question, let alone the possible difference. The MIN_POSITIVE tests aren't going to be effective there either, and actually their exponent part will be less than MIN_EXP already due to the mantissa offset -- not sure if that will work out via subnormal temporaries or not.