Ring arithmetic bug fix for RS decomposition #8636
Conversation
|
Hello. You may have forgotten to update the changelog!
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #8636 +/- ##
==========================================
- Coverage 99.43% 99.42% -0.02%
==========================================
Files 587 587
Lines 61887 61971 +84
==========================================
+ Hits 61538 61615 +77
- Misses 349 356 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| ( | ||
| ZOmega(-22067493351, 22078644868, 52098814989, 16270802723), | ||
| ZOmega(-21764478939, 70433513740, -5852668010, 4737137864), | ||
| 73, |
There was a problem hiding this comment.
is this the same or different example from the paper? I thought that one had 72?
There was a problem hiding this comment.
It's an example you send in the chat. I computed 73 based on its float value.
| (ZOmega(d=28), ZOmega(d=12), ZOmega(d=4)), | ||
| (ZOmega(d=15), ZOmega(d=25), ZOmega(d=5)), | ||
| (ZOmega(d=81), ZOmega(d=63), ZOmega(d=9)), | ||
| (ZOmega(d=144), ZOmega(d=108), ZOmega(d=36)), |
There was a problem hiding this comment.
can you add some tests for cases where a/b/c are not 0? maybe you can just find two primes A, B, and multiply some unit U in ZOmega and make it find U = gcd(AU,BU)?
There was a problem hiding this comment.
Added a few of them. I can add more if you'd like.
| return ZSqrtTwo(self.a % int(other), self.b % int(other)) | ||
|
|
||
| d = abs(other) | ||
| if (s := abs(self)) < (d := abs(other)): |
There was a problem hiding this comment.
I'm trying to convince myself about this - is there any proof/reference for this?
There was a problem hiding this comment.
I have no proof. I was simply trying to accommodate for the time when a < b. That's why i asked you to benchmark this separately. 🤔
There was a problem hiding this comment.
Essentially, it is a matter of convention to be honest about whether you would allow for a negative modulus.
There was a problem hiding this comment.
if I have a = 3*sqrt(2) , b = sqrt(2), we should have a%b = 0.
But we have abs(a) = -18, and abs(b) = -2, abs(a) < abs(b) is true, and this will return a, which I don't think is correct?
There was a problem hiding this comment.
I don't think abs orders the elements in ZSqrtTwo?
There was a problem hiding this comment.
true. I think for them it is easier to use the float values instead.
Context: This fixes GCD and modulo computations for the
ZSqrtTwoandZOmegaring elements.Description of the Change:
Benefits:
Possible Drawbacks:
Related GitHub Issues: