Implement abs, ceil, copysign, Rem, to_radians, to_degrees, and trunc#6
Implement abs, ceil, copysign, Rem, to_radians, to_degrees, and trunc#6TimTheBig wants to merge 4 commits into
abs, ceil, copysign, Rem, to_radians, to_degrees, and trunc#6Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several mathematical utility methods to the Rational and Real types, including ceil, trunc, abs, copysign, and atan2. It also implements the remainder operator (%) for both types and adds specialized conversion methods between radians and degrees (to_degrees, to_radians). Feedback highlights critical issues regarding mathematical correctness for irrational values in ceil, trunc, and abs, which currently only round the rational scale and cause cache inconsistencies. Furthermore, the to_degrees and to_radians implementations violate internal invariants by failing to ensure that rational scales remain reduced after direct modification.
| pub fn ceil(mut self) -> Self { | ||
| self.rational = self.rational.ceil(); | ||
|
|
||
| map_cache!(self.primitive_approx_cache, num::Float::ceil); | ||
|
|
||
| self | ||
| } |
There was a problem hiding this comment.
The implementation of ceil on Real is mathematically incorrect for non-rational values (classes other than One). It currently performs a "scale-wise" ceiling, which does not correspond to the ceiling of the represented value. For example, Real::pi().ceil() returns pi (approx 3.14) instead of 4, and (Real::pi() * 0.5).ceil() returns pi instead of 2. If an exact ceiling cannot be computed for irrationals, this method should either be restricted to rational Reals or return a Computable representing the ceiling. Furthermore, the map_cache! call here introduces a critical bug: it updates the cache with the float ceiling of the entire value, while the Real itself is only updated scale-wise, leading to an inconsistent state where the cached float does not match the symbolic value.
| pub fn trunc(&self) -> Self { | ||
| use PrimitiveApproxCache::*; | ||
|
|
||
| // ? do computable need to be run first | ||
| Self { | ||
| rational: self.rational.trunc(), | ||
| // it's over one (int) and rational | ||
| class: Class::One, | ||
| computable: None, | ||
| signal: None, | ||
| primitive_approx_cache: Cell::new(match self.primitive_approx_cache.get() { | ||
| #[cfg(feature = "cached-f32-approx")] | ||
| F32(f) => F32(f.map(|f| f.trunc())), | ||
| #[cfg(feature = "cached-f64-approx")] | ||
| F64(f) => F64(f.map(|f| f.trunc())), | ||
| _ => Empty, | ||
| }), | ||
| } | ||
| } |
There was a problem hiding this comment.
The implementation of trunc on Real suffers from the same mathematical incorrectness as ceil for non-rational values. For example, Real::pi().trunc() returns 1 instead of 3. Additionally, it is inconsistent with ceil because it forces the result to Class::One, effectively discarding the symbolic part entirely after truncating the scale. Like ceil, the use of trunc() on the primitive cache while only truncating the scale in the symbolic representation leads to cache inconsistency (e.g., trunc(pi) would have a rational value of 1 but a cached float of 3.0).
| pub fn abs(mut self) -> Self { | ||
| self.rational = self.rational.abs(); | ||
|
|
||
| map_cache!(self.primitive_approx_cache, num::Float::abs); | ||
|
|
||
| self | ||
| } |
There was a problem hiding this comment.
The abs method is incorrect for the Irrational class. It only ensures the rational scale is positive, but for an opaque irrational, the underlying Computable might be negative. While the docstring acknowledges this, providing a method named abs that returns a negative value is highly misleading and likely to cause bugs in generic code. This should at least attempt to use best_sign() to determine if a negation is needed, or return a Result if the sign is unknown.
| self.rational.numerator *= BigUint::from(180u64); | ||
| self.class = One; | ||
| self.computable = None; | ||
| self.signal = None; | ||
| self.primitive_approx_cache = Cell::new(PrimitiveApproxCache::Empty); | ||
| return self; |
There was a problem hiding this comment.
Directly multiplying the numerator of the internal Rational breaks the reduced fraction invariant. If self.rational was 1/2, it becomes 180/2, which is not reduced. This causes predicates like is_integer() (which checks if the denominator is 1) to return incorrect results. You should use the * operator or call .reduce() after the modification.
self.rational = self.rational * Rational::new(180);
self.class = One;
self.computable = None;
self.signal = None;
self.primitive_approx_cache = Cell::new(PrimitiveApproxCache::Empty);
return self;| self.rational.denominator *= BigUint::from(180u64); | ||
| self.class = Pi; | ||
| self.computable = Some(Computable::pi()); | ||
| self.primitive_approx_cache = Cell::new(PrimitiveApproxCache::Empty); | ||
| return self; |
There was a problem hiding this comment.
Directly multiplying the denominator breaks the Rational reduced invariant. For example, to_radians(180) would result in a rational scale of 180/180, which is not reduced to 1/1. This will cause is_one() and other structural checks to fail. Use the / operator instead.
self.rational = self.rational / Rational::new(180);
self.class = Pi;
self.computable = Some(Computable::pi());
self.primitive_approx_cache = Cell::new(PrimitiveApproxCache::Empty);
return self;
2 participants
Most of the diff is tests