Adding __reduce__ method to QuadPrecision for supporting pickle#105
Adding __reduce__ method to QuadPrecision for supporting pickle#105SwayamInSync wants to merge 6 commits into
__reduce__ method to QuadPrecision for supporting pickle#105Conversation
ngoldbaum
left a comment
ngoldbaum
left a comment
There was a problem hiding this comment.
I asked claude to review this and it spotted some issues (unverified by me):
- The parsing relies on
Sleef_strtoq, which Claude identified as not being lossless for the decimal case:sleef/src/quad/sleefsimdqp.c:3504-3546. Only hex-encoded values are losslessly converted. - You chose to serialize to the shortest representable form, but that leaves no slack for rounding error.
Elsewhere in the codebase you use quad_to_string_same_value_check to guard against this possibility but didn't use it in the new __reduce__ codepath, so it's possible to write values to disk that can't be round-tripped.
The new tests don't check places where the rounding error would be particularly bad: subnormals like 6.5e-4966 and values near the maximum representable value.
It also found a pre-existing issue in the dragon4 implementation: the hasUnequalMargins flag is computed after the mantissa bit it is OR'd in, so it's always false. It suggests this instead:
diff --git a/src/csrc/dragon4.c b/src/csrc/dragon4.c
index f86f269..405d751 100644
--- a/src/csrc/dragon4.c
+++ b/src/csrc/dragon4.c
@@ -1925,7 +1925,7 @@ Dragon4_PrintFloat_Sleef_quad(Sleef_quad *value, Dragon4_>
/* factor the value into its parts */
if (floatExponent != 0) {
/* normal */
- mantissa_hi = (1ull << 48) | mantissa_hi;
+ mantissa_hi = (1ull << 48) && mantissa_lo == 0;
/* mantissa_lo is unchanged */
exponent = floatExponent - 16383 - 112;
mantissaBit = 112;
That's a pre-existing issue though and I haven't confirmed it.
It also generated this script which finds values that don't round-trip using several different methods: https://gist.github.com/ngoldbaum/0bbe4ed5d2479a76885b3f3c2ed7233a#file-rt_hunt-py
| const char *scientific_str = Dragon4_Scientific_QuadDType_CStr(sleef_val, DigitMode_Unique, | ||
| SLEEF_QUAD_DECIMAL_DIG, 0, 1, | ||
| TrimMode_LeaveOneZero, 1, 2); | ||
| TrimMode_LeaveOneZero, 1, 4); |
There was a problem hiding this comment.
exp_digits is the minimum width of the exponent field (zero-padded).
Dragon4 always emits every significant exponent digit regardless, so this has no effect on round-trip correctness.
e+4932 prints in full at exp_digits=2 just as at 4.
The change is purely for consistency width across all formatting paths __repr__ and __reduce__ already use exp_digits=4, and binary128 exponents span up to 4 digits (±4932), so a 4-digit field formats every value consistently. casts.cpp was the only site still at 2, which produced variable-width exponents (...e+05 vs ...e+0005) for the same dtype depending on code path so setting it to 4 makes the scientific output uniform everywhere.
|
|
||
| static PyObject * | ||
| QuadPrecision_str(QuadPrecisionObject *self) | ||
| // This is just define here for debugging, we actually use QuadPrecision_str_dragon4 for __str__ method. |
There was a problem hiding this comment.
which means it's dead code, right? why not delete it? If you really want to use it for debugging then expose a private interface for it, IMO
There was a problem hiding this comment.
yup this and its repr cousin both, will delete them in this PR
|
|
||
| @pytest.mark.parametrize("backend", ["sleef", "longdouble"]) | ||
| def test_pickle_scalar_issue_repro(self, backend): | ||
| """The exact repro from #99: was RuntimeError on loads().""" |
There was a problem hiding this comment.
This sort of comment isn't very helpful, IMO the test name is enough. I'd adjust your prompting to encourage the model to not comment like this.
| assert loaded[3] == original[3] | ||
|
|
||
| @pytest.mark.parametrize("backend", ["sleef", "longdouble"]) | ||
| def test_pickle_scalar_loads_does_not_raise(self, backend): |
There was a problem hiding this comment.
Isn't this test doing the same thing as test_pickle_scalar_issue_repro? I also think it's kind of silly to do pytest.fail in an except block: the test will fail if an exception is raised either way
| """Diagnostic precision-loss test. Two values with the same printed | ||
| repr can still differ at the full bit width, so check via subtraction | ||
| (which preserves precision) — `loaded - original` must be exactly zero. | ||
| Catches the regression where the longdouble path went through (double).""" |
There was a problem hiding this comment.
I'd delete this last line too - this sort of detail about the code's history or historical bugs shouldn't show up in comments IMO - LLMs tend to generate comments like this.
|
The dragon4 roundtrip issue was actually real (means might have to update NumPy upstream too) Reason this kept ignored even having roundtrip test-cases is because none of them purely targets the power of 2s. We only checked the subnormal, max and in between ones. |
|
Let me merge #104 since it is approved and update here |
|
Hey @ngoldbaum if you get some time have an eye on this again. I mostly resolved all the reviews and the discovered bug. |
2 participants
closes #99
Fixing
exp_digits=4(since float128 can have max exponent of 4932) at each place of Dragon4 call to keep consistency in codebase