Skip to content

Fix/atol rtol alignment - #262

Closed
m-messer wants to merge 2 commits into
mainfrom
fix/atol-rtol-alignment
Closed

m-messer wants to merge 2 commits into
mainfrom
fix/atol-rtol-alignment

Conversation

@m-messer

@m-messer m-messer commented Jun 2, 2026

Copy link
Copy Markdown
Member

Problem: The atol and rtol tolerance checks in the symbolic and physical quantity evaluators used inconsistent, non-standard logic: they treated atol and rtol as separate independent conditions (both had to pass), used the answer as the rtol reference in some places and the response in others, and used strict < rather than <=.

Solution: Align both evaluators with NumPy's isclose formula: |ans - res| <= atol + rtol * |ans|, where ans is the reference value. This combines atol and rtol into a single tolerance bound, matches the behaviour users familiar with NumPy would expect, and is consistent with isSimilar in other tools.

Changes:

  • app/context/symbolic.py: Replaced the separate atol/rtol checks with the combined NumPy formula. The division-by-ans form (|(ans-res)/ans| <= rtol + atol/|ans|) is used to handle expressions with symbols (e.g. rho*L^3). Falls back to rtol-only tolerance if |ans| itself cannot be evaluated (e.g. answer is still partially symbolic).
  • app/context/physical_quantity.py: Replaced the rtol-or-atol branching logic with the same combined NumPy formula. The direct form is used here since the is_constant() guard guarantees both sides are fully numeric before the comparison runs.

@m-messer

Copy link
Copy Markdown
Member Author

Closing in favour of #289, which makes the same change (the numpy.isclose rule, |ans - res| <= atol + rtol*|ans|) on the refactored code, using one shared check for the symbolic and physical quantity contexts.

@m-messer m-messer closed this Oct 19, 2026
@m-messer
m-messer deleted the fix/atol-rtol-alignment branch October 19, 2026 20:18
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