Implement num_traits::Float trait for Df64 - #15
Conversation
Implement the num_traits::Float trait to provide standard floating point operations interface for Df64 type. Changes: - traits.rs: Add Float trait implementation with 58 methods using delegation design pattern. Add 5 new tests for Float trait methods. Update 4 locations to use explicit From<f64> type syntax. - convert.rs: Add NumCast trait implementation. Note: cbrt() and integer_decode() are marked as todo!() due to requiring additional design for double-double representation. Closes tuwien-cms#2
|
Thank you for the contribution. I have a few comments:
|
This reverts commit ee2aba0. Rolling back the previous Float trait implementation to address PR review feedback and implement a better design approach. Reasons for revert: - Float trait should not depend on ComplexField trait (more basic trait should not depend on more complex trait) - Need to refactor implementation to use only basic operations Will re-implement Float trait without ComplexField dependency in the next commit. Related to PR tuwien-cms#15
Implement the num_traits::Float trait to provide standard floating-point operations for Df64 without depending on higher-level traits. Changes: - traits.rs: Add Float trait implementation with 58 methods delegating directly to low-level modules (checks, round, exp, circular, hyperbolic, funcs, arith, roots, consts) instead of ComplexField - convert.rs: Add NumCast trait implementation to support generic numeric type conversions Design: - Float and ComplexField are now independent, both delegating to the same low-level modules - All methods use #[inline(always)] for performance - cbrt() and integer_decode() marked as todo!() pending double-double specific design Addresses PR tuwien-cms#15 review feedback from @shinaoka
Add 56 unit tests covering all 58 methods of the num_traits::Float trait implementation for Df64, ensuring API correctness and mathematical properties. Test coverage: - Constants (8 methods): nan, infinity, epsilon, min/max values - Classification (7 methods): is_nan, is_infinite, is_finite, is_normal, classify - Basic arithmetic (3 methods): abs, signum, recip - Rounding (5 methods): floor, ceil, round, trunc, fract - Comparison (5 methods): min, max, abs_sub, mul_add, copysign - Powers & roots (3 methods): powi, powf, sqrt, hypot - Exponential & logarithmic (8 methods): exp, exp2, ln, log, log2, log10, exp_m1, ln_1p - Trigonometric (8 methods): sin, cos, tan, asin, acos, atan, atan2, sin_cos - Hyperbolic (6 methods): sinh, cosh, tanh, asinh, acosh, atanh - Angle conversion (2 methods): to_degrees, to_radians - Not yet implemented (2 methods): cbrt, integer_decode (panic tests) Test characteristics: - Each test covers positive, negative, and edge cases (zero, infinity, NaN) - Validates mathematical properties (e.g., odd/even functions, identities) - Uses high precision tolerance (1e-30 typical, relaxed to 1e-20~1e-29 where needed) - Special validations: tan(π/2) divergence, atan(∞) convergence All 94 tests pass (57 Float trait tests + 37 existing implementation tests). Related to PR tuwien-cms#15
|
@shinaoka Thank you for the review. I've addressed both of your comments: 1. Removed ComplexField dependencyThe Float trait implementation now delegates directly to low-level modules instead of ComplexField:
See commit: b0db211 2. Added comprehensive unit testsAdded 56 unit tests covering all 58 Float trait methods:
All 94 tests pass (57 new Float trait tests + 37 existing tests). See commit: 2163c11 Summary of changes
Ready for re-review. Please let me know if you have any other concerns. |
Integrate +0.0/-0.0 behavior tests into existing Float trait tests and add dedicated tests for signed zero arithmetic and copysign operations.
|
@shinaoka I've added two more commits addressing additional issues found during review: 1. Fix
|
There was a problem hiding this comment.
Thanks for the PR! Looks good, but the tests need work.
- Please do not use check against an absolute threshold. Most functions satisfy a relative accuracy goal
- Please do not use magic numbers (
1e-29etc.) in the threshold. Use a small multiple of the machine epsilon - Please reuse the functions in
test_utilsrather rolling your own tests whenever useful. Otherwise, please make use of the macros inside theapproxcrate, in particularapprox::assert_relative_eqandassert_ulps_eq.
In general, most of the tests seem to duplicate the unit tests in the respective modules, for example, exp2 and so forth. Why test again and in a separate place?
| #[inline(always)] | ||
| fn mul_add(self, a: Self, b: Self) -> Self { | ||
| // There are two requirements that one has with fma: (1) it must be | ||
| // accurate without intermediate rounding and (2) it must be at least | ||
| // as fast as (a*b)+c. We have no way of satisfying both, so we go | ||
| // for performance. | ||
| (self * a) + b | ||
| } |
There was a problem hiding this comment.
This duplicates an existing method, please unify.
| #[inline(always)] | ||
| fn to_degrees(self) -> Self { | ||
| self * consts::ONE_OVER_PI * <Df64 as From<f64>>::from(180.0) | ||
| } | ||
|
|
||
| #[inline(always)] | ||
| fn to_radians(self) -> Self { | ||
| self * consts::PI / <Df64 as From<f64>>::from(180.0) | ||
| } |
There was a problem hiding this comment.
Please use precomputed values for PI/180 and its reciprocal here.
…an conversion - Add mul_add_qq function to arith.rs for fused multiply-add on Df64 - Add RADIANS_PER_DEGREE and DEGREES_PER_RADIAN constants to consts.rs - Update Float::mul_add and ComplexField::mul_add to delegate to arith::mul_add_qq - Update to_degrees/to_radians to use precomputed constants instead of runtime computation Addresses PR review feedback from mwallerb
…agic numbers - Replace absolute thresholds (1e-30, 1e-29, etc.) with assert_ulps_eq! macro - Use Df64::EPSILON-based comparisons for near-zero checks - Tests now use relative precision based on machine epsilon - Add comments clarifying that detailed precision tests are in respective modules - Float trait tests focus on verifying correct delegation to low-level modules Addresses PR review feedback from mwallerb regarding test quality
- Add is_subnormal() method delegating to checks::is_subnormal() - Add clamp() method delegating to funcs::clamp() - Add corresponding unit tests
- Fix Basic arithmetic comment: 5 -> 3 methods - Fix Trigonometric functions comment: 7 -> 8 methods - Fix test section name: "special constants" -> "not yet implemented" - Move test_float_powi and test_float_powf to exponential section
|
@mwallerb Thank you for the detailed review. I've addressed all your feedback in the following commits: 1. Test improvements (1c4a7cb)
Regarding the test duplication concern: you're right that some tests appear similar to those in the underlying modules. The Float trait tests are intentionally minimal now - they verify that delegation works correctly rather than re-testing mathematical accuracy. 2. Unified
|
|
@mwallerb |
This PR implements #2.
Summary
This PR implements the
num_traits::Floattrait forDf64, enabling compatibility with generic numeric code that relies on this standard trait. Additionally, theNumCasttrait is implemented to support generic numeric type conversions.Problem
As requested in #2, the
Df64type lacked thenum_traits::Floattrait implementation, which prevented access to essential floating-point inspection methods likeis_infinite(),epsilon(),is_nan(), andclassify(). This limited interoperability with Rust's numeric ecosystem and generic code that depends on these trait bounds.Solution
traits.rs- Float trait implementationnum_traits::Floatmethods forDf64Df64constants (NAN,INFINITY,EPSILON, etc.)checksmodule functionsSignedtraitComplexFieldtraitRealFieldtraitComplexFieldtraitComplexFieldtraitComplexFieldtraitto_degrees()andto_radians()implemented directly using PI constantcbrt()andinteger_decode()marked astodo!()with explanatory comments<Df64 as From<f64>>::from()instead ofDf64::from()for consistencytest_float_constants()- Verify Float constant methodstest_float_neg_zero()- Verifyneg_zero()implementationtest_float_checks()- Verify classification methods (is_nan(),is_infinite(), etc.)test_float_classify()- Verifyclassify()methodtest_float_conversions()- Verifyto_degrees()andto_radians()convert.rs- NumCast trait implementationnum_traits::NumCasttrait to enable generic numeric type conversionsFromPrimitive::from_f64()implementationChanges
Floattrait implementation (+326 lines)From<f64>syntaxNumCasttrait implementation (+6 lines)Testing
traits.rsverify:nan(),infinity(),epsilon(), etc.)is_nan(),is_infinite(),is_finite(), etc.)neg_zero(),classify())to_degrees(),to_radians())Future Work
ComplexField::cbrt()and subsequentlyFloat::cbrt()using Newton iteration or other stable algorithmsDf64::integer_decode()with appropriate representation for double-double mantissa (possibly using extended mantissa format or custom representation)