Skip to content

Add approx_rational for rational approximations of Computables#11

Open
TimTheBig wants to merge 4 commits into
timschmidt:mainfrom
TimTheBig:approx_rational
Open

Add approx_rational for rational approximations of Computables#11
TimTheBig wants to merge 4 commits into
timschmidt:mainfrom
TimTheBig:approx_rational

Conversation

@TimTheBig

Copy link
Copy Markdown

No description provided.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request introduces the approx_rational method to Computable in src/computable/node.rs, which provides a dyadic rational approximation within a specified precision. It includes a fast-path optimization for exact-rational leaves with power-of-two denominators and adds comprehensive unit tests. The feedback suggests simplifying the fast-path check by reusing the existing dyadic_denominator_shift method on Rational to improve code maintainability.

Comment thread src/computable/node.rs Outdated
@timschmidt

timschmidt commented Jun 19, 2026

Copy link
Copy Markdown
Owner

What is the goal here?

Real::exact_rational_or_approx* ignores Real.rational for non-One classes. In src/real/arithmetic.rs:1352 and src/real/arithmetic.rs:1385, the code approximates only the stored/computed class payload. But Real is rational * class, and many symbolic constants are scaled this way, for example Real::tau() is 2 * Pi with a Computable::pi() payload. The PR returns 201/64 for Real::tau().exact_rational_or_approx(-6), but the correct result is 201/32. Same bug affects borrowed form, pi/2, scaled logs/sqrts, and generic symbolic scaled values.

Real.fold() already manages approximation at the level of Real, which is what you need to correctly address the above issue.

@TimTheBig

TimTheBig commented Jul 2, 2026

Copy link
Copy Markdown
Author

What is the goal here?

The goal is to provide both convenience and correctness. I could see others making this function so why not make an internal one that takes advantage of shortcuts.

As for why you would need it, sometimes you need a rational-only operation that an f64 would be to imprecise or small for.

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