Skip to content

Add a test of utils::factorial#19

Merged
Axect merged 7 commits intoAxect:v0.3.0from
JSorngard:v0.3.0
Aug 18, 2024
Merged

Add a test of utils::factorial#19
Axect merged 7 commits intoAxect:v0.3.0from
JSorngard:v0.3.0

Conversation

@JSorngard
Copy link
Copy Markdown
Contributor

@JSorngard JSorngard commented Aug 14, 2024

This PR adds a test of the factorial function in puruspe, by calculating the factorial naively in a loop and comparing with the output. I use approximate comparisons because there are different floating point errors at very large outputs.

This adds tests for one of the utils functions for #12.

@JSorngard
Copy link
Copy Markdown
Contributor Author

JSorngard commented Aug 14, 2024

It is possible to test to larger input values, but the exponents on the outputs are so large that the small differences in the mantissa due to different floating point errors between the function and the test code result in values that differ enough to need custom epsilon values.

@JSorngard
Copy link
Copy Markdown
Contributor Author

I added a big test case manually

Copy link
Copy Markdown
Owner

@Axect Axect left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for adding test for the factorial function.. I've reviewed the changes, and I'm pleased to approve this PR. Here are my thoughts:

  1. The test coverage for inputs from 0 to 90 is comprehensive and uses appropriate approximate comparisons to account for floating-point errors.

  2. Including a test case for the largest factorial that fits in an f64 (170!) is an excellent addition. This edge case is important to verify.

  3. Your explanation about the limitations of testing larger input values due to floating-point precision issues is clear and reasonable.

  4. The code is well-written and easy to understand.

  5. This contribution successfully addresses part of the testing requirements for #12.

Great job on implementing these tests. They will certainly improve the reliability of our factorial function. I'm merging this PR now. Thank you for your valuable contribution to the project!

@Axect Axect merged commit b8948aa into Axect:v0.3.0 Aug 18, 2024
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.

2 participants