Repository navigation
fix: handle negative imaginary parts in number_to_string - #616
Sanjays2402 wants to merge 1 commit into
Conversation
seperman
left a comment
There was a problem hiding this comment.
Static review: #616 fix: handle negative imaginary parts in number_to_string
Summary: Fixes the "1.0+-1.0j" malformed-literal ValueError from #551. In the complex branch of number_to_string, the + separator is now omitted when the rendered imaginary part already starts with -. The fix is correct, but the same fix is already on dev.
Overlap with other PRs
- #598 (merged into
dev, also fixes #551) makes a functionally identical change. It computesrealandimagseparately and writesimag = imag if imag.startswith("-") else "+" + imag(helper.py:497-513ondev). This PR'ssign = '' if str(imag).startswith('-') else '+'(helper.py:511) gives the same output for every input. I found no other open PR for #551. - This PR targets
master, which doesn't have #598 yet. Againstdevit conflicts indeepdiff/helper.pyand inCHANGELOG.md, wheredevalready has its ownUnreleasedsection.tests/test_helper.pymerges cleanly.
Security
No security concerns found in static review. The change is a local string-formatting refactor plus test cases and a changelog line. There is no I/O, eval or dependency change, and the diff has no non-ASCII or hidden characters.
Correctness
deepdiff/helper.py:499-514: the logic is correct.-0.0imaginary parts are normalised by the recursive call'sabs()for zero.nan/-infimaginary parts also produce parseable strings.str(imag)athelper.py:511does nothing, becauseimagis already the string returned bynumber_to_string.
Tests
- The four new cases (
tests/test_helper.py:187-190) would fail onmaster. Checked by hand againstdev's implementation, they should pass there already. Two of them cover inputs #598's tests don't:"e"notation (1.0e+0-1.0e+0j) and a negative real with a negative imaginary part (-1.0-1.0j). #598 only added"f"-notation cases with a positive real part.
Suggestions
- Since the code fix is already on
devvia #598, the most useful remaining part is the extra test coverage. A small PR againstdevthat adds only the"e"-notation and negative-real cases totest_number_to_string_complex_digitswould keep that coverage without conflicts. - Please target
devfor future PRs, and leaveCHANGELOG.mdedits to the maintainer at release time.
AI-assisted static review; the code was not executed.
Adds the two cases from the original fix that are not covered on dev: - 'e' notation with negative imaginary part (1.0e+0-1.0e+0j) - negative real with negative imaginary part (-1.0-1.0j) The code fix itself is already on dev via qlustered#598; this keeps only the extra test coverage. Targets dev, no CHANGELOG edit.
3e49e55 to
43da3ef
Compare
|
Thanks for the review! You're right, the code fix is redundant with #598. I've reworked this into a test-only PR against dev: dropped the helper.py change and the CHANGELOG edit, and kept just the two cases #598 doesn't cover (the "e"-notation case and the negative-real with negative-imaginary case). Tests pass locally. |
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #616 +/- ##
=======================================
Coverage 95.07% 95.07%
=======================================
Files 18 18
Lines 4918 4918
=======================================
Hits 4676 4676
Misses 242 242 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Closes #551
number_to_stringjoined the rendered real and imaginary parts with a hardcoded+, so a negative imaginary part produced a malformed literal like1.0+-1.0j, whichcomplex()rejects —number_to_string(1-1j, significant_digits=1)raisedValueError, and so did any diff/hash comparison involving such a number. The separator is now omitted when the rendered imaginary part is already negative.Four cases added to the existing
test_number_to_string_complex_digitsparametrize list intests/test_helper.py; they fail onmasterand pass with the fix.This change was prepared with AI assistance; the regression test was run locally and fails without the fix.