Repository navigation
Floor and clamp the exponent of a root of zero - #153
Conversation
rootSpecials divided the operand exponent by factor with Go's /, which truncates toward zero, so every odd negative exponent came out one too high. It also never brought the result into the context's exponent range and never raised Clamped, unlike quoSpecials. TestGDA compares finite results with Cmp, which treats every zero as equal regardless of exponent, so the vendored vectors never caught it.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 06d21033f3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if x.Exponent%factor != 0 && x.Exponent < 0 { | ||
| e-- |
There was a problem hiding this comment.
Because rootSpecials is also called by Cbrt, flooring here changes positive cube-root results with negative, non-divisible exponents: for example, Cbrt("00.0") now returns 0.0 with exponent -1, while testdata/cuberoot-apd.decTest cases cbtx006–cbtx008 require exponent 0. TestGDA does not expose this regression because it compares zeros numerically, and the new targeted test only selects the contradictory negative-zero expectations. Restrict the flooring change to square root, or first define and consistently update and test the cube-root contract.
Useful? React with 👍 / 👎.
cuberoot-apd.decTest expected exponent 0 for the cube root of 00.0, 0.00, 00.00 and 0E+5 but exponent -1 for -0.00 and -00.0. The floor rule gives -1, -1, -1 and 1, matching the negative rows. Fix the four rows and add vectors for a cube root of zero that clamps under a narrow exponent range. The vectors now cover every exponent case in zeroroot_test.go, and TestGDA already runs each vector with the result aliased to the operand, so replace that file with a test of the Traps path, which a vector cannot express. Note in rootSpecials that the cube root rule is apd's convention, since cube root is not a GDA operation. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
TestGDA compared finite results with Cmp, which treats every zero as equal regardless of exponent, so a zero result with the wrong exponent passed. Compare the exponent too when the result is zero. Vectors for other operations that fail this check are listed in GDAignoreZeroExponent with the mismatch, so their value and flag checks still run. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
rafiss
left a comment
rafiss
left a comment
There was a problem hiding this comment.
The rootSpecials change is right: it floors the exponent, clamps to [Etiny, MaxExponent], and raises Clamped, matching Python's decimal and the squareroot vectors. I verified it independently and the full suite passes with the sqtx ids un-ignored.
I pushed two commits on top so this can land as one piece:
6bcbdd8fixes cbtx006/007/008/022 in cuberoot-apd.decTest, which expected exponent 0 and contradicted cbtx015/016 in the same file, and adds cbtx910-923 covering a clamped cube root of zero under a narrow exponent range. It replaces zeroroot_test.go with a small Traps test in decimal_test.go, since the vectors now cover the exponent cases and TestGDA already runs each vector with the result aliased to the operand. It also notes in rootSpecials that the cube root rule is apd's convention, as cube root is not a GDA operation.8e220fbmakes TestGDA compare the exponent of a zero result, so the bug this PR fixes would have been caught by the vendored vectors. The 32 pre-existing mismatches in plus, reduce, tointegral and tointegralx are listed in GDAignoreZeroExponent; only the exponent check is skipped for them, and they are worth a follow-up issue.
CI is green on all 18 jobs. Approving.
SqrtandCbrtof a zero return the wrong exponent and never clamp.The repo's own vendored vector disagrees —
testdata/squareroot.decTest:3780sayssqtx9010 squareroot 0E-9 -> 0.00000, exponent −5, and Python'sdecimalagrees. Every odd negative exponent is off by one. The clamp is missing outright: withMaxExponent: 9,Sqrtof0E+100returns exponent 50 and noClamped.rootSpecials(context.go:462-465) doesd.Exponent /= factor, and Go's/truncates toward zero where the ideal exponent isfloor(e/factor); the result is then never brought into[etiny, MaxExponent]nor flagged. The siblingquoSpecials(context.go:247-250) already does both. I usedc.etiny()andc.goErrorrather thansetExponent, becausesetExponentalso raisesRounded, which these vectors do not expect.Why 22,915 conformance cases never caught it:
gda_test.go:714-715compares finite results withd.Cmp(r), which is numeric, so every zero compares equal to every other zero regardless of exponent. The decTest files carry the exponents; the comparison discards them.One thing to decide: Cbrt is collateral
rootSpecialsis shared, so this changesCbrttoo, andtestdata/cuberoot-apd.decTest— this repo's own hand-written file — is already inconsistent about zeros:Same operand exponent, contradictory expectations;
cbtx022 cuberoot 0E+5 -> 0also disagrees withcbtx017/cbtx020. None of it is enforced, for the sameCmpreason. I did not touch that data. Happy to correct those lines in a follow-up, or to restrict this tofactor == 2and leaveCbrtalone — your call.Verification
mastersqtx006x.Exponent < 0test)sqtx9037Clampedflagsqtx9024,got conditions "", expected "clamped"This lets 7 entries out of
GDAignoreFlags, checked both ways:TestGDAexits 0.TestGDAfails on exactly those 7 IDs. So the removals are earned by the fix, not by loosening the test.sqtx9045stays: removing it too still fails, because apd does not implement theclamp:directive. I corrected its comment to say so.CI rows on go1.26.3:
go test ./...,-race,-bench=. -benchtime=1x,go vet -unsafeptr=false ./...,staticcheck ./..., andgo test -cforGOARCH=arm GOARM=7andarm64— all exit 0. I did not run a real go1.17/1.19 toolchain; the diff uses only/,%and existing identifiers.gofmt -l .flags four files on pristine too (go1.26's gofmt reformats the//gcassert:comments), so I did not rungofmt -w; the diff contains only therootSpecialshunk.Disclosure: I used Claude (an AI assistant) while preparing this change. Every result above I ran and verified myself.