feat(float): implement arbitrary-precision trigonometric functions and constants - #60
Conversation
|
Thanks for the PR! I will try to find a time to review and merge 👍 |
|
I tried to follow more or less what was there already if you don't agree with any design decision just ask for the changes |
|
Just a note I forgot to leave there are still some panick paths that I left that in principle should never happen but if you want to discuss how to handle them I will patche to crate design preferences |
fb8c49a to
adce916
Compare
|
Update: Enhanced Precision Stability & Performance Optimizations I've just pushed a series of updates to address some edge-case precision issues and optimize memory usage:
The implementation is now significantly leaner and more robust against extreme scale inputs. |
|
Any attention needed here from my part? |
|
I still need sometime to review the whole PR, I do have some feedback from Deepseek, I do agree with its suggestions: Issues and Suggestions
float/Cargo.toml:64 adds rug = "1.24" which depends on GMP/MPIR via gmp-mp-fr-sys. This is a native C dependency that:
Consider gating behind a feature flag (e.g., dev-utils) so it's only compiled when explicitly needed.
consts.rs:33: (self.precision as f64 * log2_b_ub as f64) as usize + 1 uses f64 to compute ceil(precision * log2(B)). This could be off by 1 for large
trig.rs:273-289 — tan delegates to sin_cos which calls compute_work_context using self.precision. If self.precision == 0, compute_work_context produces
sin, cos, and sin_cos each contain ~20 nearly identical lines for:
This should be extracted into a helper like reduce_to_quadrant(x) -> (FBig, i8) to avoid the duplication and risk of divergence.
trig.rs:782-823 — The infinity handling in atan2 has 4 levels of nesting. Consider a lookup table or early-return pattern:
trig_random.rs:330-331 — Uses FBig::from_repr(s_r.value(), dashu_ctx). If from_repr is not a public API, this test may break. Verify this is intentionally
trig.rs:524: let threshold = sum.sub_ulp() is computed from the initial sum (x), not from the running sum. For x near 0, this threshold is very small — good.
trig.rs:700 — Missing blank line before /// Calculate the arctangent doc comment after the closing brace of acos.
sin and cos call assert_limited_precision(self.precision) after the infinite check but before compute_work_context. If self.precision == 0, the function
trig_random.rs:1089 — mod helper_macros; is included in the test file but the random_dbig function is not #[cfg(test)] gated (it's in a test file so this is |
|
1 - So maybe gatting the fuzz tests behind some kind of feature, I never used features for dev dependencies and I don't think it is a good idea to create features users are never gona use do you recommend trying to add similar fuzz and property based testing similar to the Pythagorean identity one? there are some I can try to add with some simple expression manipulation on trig identities but i think fuzzing against a well established lib is always better as this can hide some symmetric bugs on the algorithms. 2 - ok fair I had to keep in attention that in another project I'm doing with rug and agree, didn't think it was a problem at the time. 3 - ok fair 4 - seems not that much code but fair also 5 - that one I need to see the code 6 - most of these seem mostly irrelevant I can try fixing them Tuesday as I need to study for eletromagnetism 1 and modern physics finals, any other finding let me know. |
|
Done |
|
Mb let those two slip in the middle of the other from other parts of the codebase |
|
Thanks for your help on editing. First, regarding the fuzz test, it seems that the fuzz test takes a lot of CI time. So my suggestion would be split the fuzz tests into a separate crate (just like why I have the Besides, I have run AI code review again and here's its feedback. Please review (you can directly feed them into your AI): |
|
Bro I'm not a openclaw 😭😭, at least format it in bullet points so it is readable I read every response/thinking tokens my ai outputs so I can make sure they don't deviate from my vision, and can correct them in the middle of thinking and implementation if they start to drift. Also like some of those are ok some don't make sense if the algorithm as linear convergence and I did research and didn't find a better one how the fuck does that AI want me to improve it write a full paper and proof with a new algorithm that does it, get a better LLM. |
|
@CokieMiner Sorry about that.. Well I can fix those issues later myself, but can you separate the fuzz test to a separate crate? That will make CI and local testing easier and cleaner. |
|
already did all just doing a final ceck on a detail |
2ddf0e2 to
3b7bc33
Compare
|
everything fixed and clean history |
|
Ok one of them is my bad, the other is do to fuzz workspace having rug as a dep that doesn't build in arm you will need to exclude fuzz compilation for arm targets |
f391b22 to
31351c2
Compare
|
Ok noticed main add a lot of updates so rebased on top |
cmpute
left a comment
There was a problem hiding this comment.
Thanks for your efforts, but please read these suggestions to make the coding style of the crate more consistent
|
The CI thing was broken because |
61878da to
6e00ea5
Compare
|
Any confusion left just say |
|
Also while thinking of a cache strategy, I came up with specific design that could be useful, don't know what other libs use but: After calculating pi for the first time, we cache it. Every time pi is requested again, we compare the requested precision with the stored one. If the requested precision is smaller, we simply round the cached value and return it. If it's higher, we could potentially use the cached value as a starting point to compute the missing digits, and then update the cache with this new, higher-precision value. For example for pi we: Store the final FBig and the raw Chudnovsky integers (P, Q, T, terms) alongside it. Since binary splitting is associative, if a higher precision is later requested, we only need to compute chudnovsky_bs(cached_terms, n_new) and combine with the cached (P, Q, T) using the standard recurrence, avoiding a full recomputation from scratch. For lower precisions, we just round the cached This gives three O(1)-or-better cases:
A problem you might have with this design is that the cache will probably have to be constant specific. I can try to implement it but can only do it Tuesday. |
|
@CokieMiner Yes I did think about adding a caching strategy, and I did note that in my TODOs. However, it's a little bit complex in Rust, we need to consider threading safety. The best way I can think is to associate the cache with the context. I will suggest leave it for the next PR (and for the next big version of dashu) |
| ); | ||
| }; | ||
|
|
||
| let k_mod_4_big = k.rem_euclid(IBig::from(4)); |
There was a problem hiding this comment.
actually you can directly call k.rem_euclid(4u8), iirc which directly produces an u8
There was a problem hiding this comment.
Just tried that even commited without testing, but IBig doesn't implement the RemEuclid < u8 > trait. I have to use IBig::from(4) and downcast it as a workaround.
2daeb1d to
ecdf1de
Compare
|
Well there is currently no easy way to create const UBig from u64 outside dashu-int crate, I think for the bigger constant we can stick to |
|
I think anything other than a global cache defeats the purpose of the caching, but happy to defer to your judgment on the design. |
|
I just checked and UBig::from does the optimization you asked for inside. |
ecdf1de to
dab788f
Compare
|
Any other problem you find just leave a comment. Think I responded to all questions and all details you pointed out. |
|
Thanks for your efforts on this PR! I think we can merge this now and make future improvements step by step |
|
Regarding constants caching, we can discuss in #15, BTW |
Overview
This PR introduces a comprehensive suite of trigonometric primitives and constants to the dashu-float crate. The implementation follows the library's architectural roadmap by consolidating advanced functions into a new math module and adopting a non-panicking FpResult pattern for robust error handling.
Key Features
sin,cos,tan,asin,acos,atan,atan2, and an optimized jointsin_cosevaluation.FpResult<B>(wrapping Normal, NaN, Infinite, etc.), ensuring safety for domain errors and singularities without library crashes.atan2.EstimatedLog2ensures correctness across all bases (binary, decimal, etc.).Internal Improvements
split_at_point_internalwhere intermediate results with high digit counts could trigger assertion failures for valuesFBig, with clear documentation on panic conditions and return types.Verification Results
tests/trig.rsfor all primitives and IEEE 754 infinity logic.cargo clippyand maintainedno_stdcompatibility by using pure integer and core-compliant arithmetic.Performance Note
The Chudnovsky implementation leverages dashu's fast multiplication for (M(n)\log^2 n)$ complexity. A$\pi$ to further optimize repeated calls at common precisions.
// TODOhas been added for future static caching of