feat: Decouple compare_frames tolerance defaults from diffly.testing - #64
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The default-tolerance behavior split is a user-visible change and needs a targeted regression test to lock in the intended defaults and the documented divergence between compare_frames and diffly.testing.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR updates Diffly’s default floating-point tolerances to better suit diff reporting by decoupling compare_frames/CLI defaults (stricter, abs_tol=0.0, rel_tol=1e-09) from diffly.testing defaults (kept looser for polars.testing.assert_frame_equal parity).
Changes:
- Split tolerance constants into comparison vs testing defaults and update call sites accordingly.
- Update CLI option defaults/help text to reflect the new comparison defaults.
- Rewrite tolerance documentation to explain the two default sets and the near-zero behavior with
abs_tol=0.0.
File summaries
| File | Description |
|---|---|
diffly/_utils.py |
Introduces separate constants for comparison vs testing default tolerances. |
diffly/comparison.py |
Switches compare_frames default tolerances to the new comparison defaults. |
diffly/_conditions.py |
Aligns internal float comparison expression defaults with the comparison defaults. |
diffly/testing.py |
Keeps testing APIs on the looser defaults via the new testing constants. |
diffly/cli.py |
Updates CLI defaults/imports and help text to match the new comparison defaults. |
docs/guides/features/tolerances.ipynb |
Updates tolerances guide to document the new defaults and their implications. |
docs/guides/features/testing.md |
Clarifies that diffly.testing defaults match polars.testing and differ from compare_frames. |
tests/test_performance.py |
Updates performance test imports/usages to the new comparison default constant names. |
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Oliver Borchert (borchero)
left a comment
There was a problem hiding this comment.
LGTM, thanks!
|
Hayden Anderson (@haydena7) could you fix the merge conflict? |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #64 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 13 13
Lines 1138 1140 +2
=========================================
+ Hits 1138 1140 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Closes #59.
Motivation
compare_frames, the CLI, anddiffly.testingshared one pair of tolerance defaults (abs_tol=1e-08,rel_tol=1e-05), inherited frompolars.testing.assert_frame_equal. Those defaults make sense for a test assertion that aims to be a drop-in replacement, but a diff report should bias toward flagging: withrel_tol=1e-05, a $1 change on a $100,000 value is hidden.One correction to the issue's framing: the reason
diffly.testingkeeps the looser pair is not a general claim that assertions should tolerate more but rather parity withpolars.testing.assert_frame_equal, so adoptingdifflydoesn't change which tests pass.Changes
Behavior change for
compare_framesand the CLI: defaults are nowabs_tol=0.0,rel_tol=1e-09, matchingpolars.Expr.is_closeandmath.isclose(PEP 485).diffly.testingis unchanged._utils.py: splitABS_TOL_DEFAULT/REL_TOL_DEFAULTinto*_COMPARE_DEFAULTand*_TESTING_DEFAULT; call sites updated.cli.py: help strings no longer hardcode the old values.compare_framesflags whatassert_frame_equaltolerates (1.0vs1.0 + 1e-7,0.0vs1e-17).abs_tol=0.0; testing guide cross-references it.Upgrade note: users with near-zero values that differ by cancellation noise will see them flagged; set
abs_tolfor the data's scale.