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.

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

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

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

mtilda
Copy link
Contributor

@mtilda mtilda commented Jun 27, 2024

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

  • Add unit test coverage for the trait method Float::integer_decode with 32-bit and 64-bit inputs.
  • Update doc comments to clarify how to recover inputs to Float::integer_decode.

@cuviper cuviper changed the title Add unit tests for trait method float::Float::integer_debug Add unit tests for trait method float::Float::integer_decode Jun 27, 2024
@mtilda mtilda force-pushed the mtilda/test/float-integer-decode branch from 24606e0 to ff6913b Compare June 27, 2024 20:37
@mtilda mtilda force-pushed the mtilda/test/float-integer-decode branch from 8037d4b to ecfb76b Compare June 27, 2024 20:56
@mtilda mtilda marked this pull request as ready for review June 27, 2024 20:57
@mtilda
Copy link
Contributor Author

mtilda commented Jun 27, 2024

@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.

@mtilda mtilda force-pushed the mtilda/test/float-integer-decode branch from ecfb76b to a00b3ec Compare June 27, 2024 21:04
`float::Float::integer_decode`
@mtilda mtilda force-pushed the mtilda/test/float-integer-decode branch from a00b3ec to 25eb45f Compare June 27, 2024 21:07
src/float.rs Outdated Show resolved Hide resolved
src/float.rs Outdated Show resolved Hide resolved
@cuviper
Copy link
Member

cuviper commented Jun 28, 2024

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
Copy link
Contributor Author

mtilda commented Jun 28, 2024

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
Copy link
Contributor Author

mtilda commented Jun 28, 2024

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.

@mtilda mtilda force-pushed the mtilda/test/float-integer-decode branch from 609effa to 6309a53 Compare June 28, 2024 19:32
@cuviper
Copy link
Member

cuviper commented Aug 14, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants