Add optimized is_trivial methods for zzModRing and ZZModRing #1949
Conversation
| @@ -56,13 +56,15 @@ is_unit(a::zzModRingElem) = a.parent.n == 1 ? a.data == 0 : gcd(a.data, a.parent | |||
|
|
|||
| modulus(R::zzModRing) = R.n | |||
|
|
|||
| characteristic(R::zzModRing) = ZZRingElem(modulus(R)) | |||
|
|
|||
| is_trivial(a::zzModRing) = is_unit(modulus(a)) | |||
There was a problem hiding this comment.
Is there a reason you use is_unit here but is_one in the other file?
There was a problem hiding this comment.
is_unit is mathematically correct. here it gets optimized to just checking for one as the modulus is unsigned . for the ZZ case using isone is an optimization to save 1 or 2 machine instructions checking for -1
There was a problem hiding this comment.
ah ok. thanks for the explanation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1949 +/- ##
==========================================
- Coverage 87.93% 87.89% -0.05%
==========================================
Files 99 99
Lines 36402 36404 +2
==========================================
- Hits 32010 31997 -13
- Misses 4392 4407 +15 ☔ View full report in Codecov by Sentry. |
| @@ -56,13 +56,15 @@ is_unit(a::ZZModRingElem) = a.parent.n == 1 ? iszero(a.data) : isone(gcd(a.data, | |||
|
|
|||
| modulus(R::ZZModRing) = R.n | |||
|
|
|||
| characteristic(R::ZZModRing) = modulus(R) | |||
|
|
|||
| is_trivial(a::ZZModRing) = is_one(modulus(a)) # constructor ensures the modulus is > 0 | |||
There was a problem hiding this comment.
perhaps this makes it clearer?
| is_trivial(a::ZZModRing) = is_one(modulus(a)) # constructor ensures the modulus is > 0 | |
| is_trivial(a::ZZModRing) = is_one(modulus(a)) # this is correct because the constructor ensures the modulus is > 0 |
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
These checks compile down to just a few machine code instructions and zero allocations. Nice.