Jaa/is nilpotent #1974
Jaa/is nilpotent #1974
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1974 +/- ##
=======================================
Coverage 87.98% 87.98%
=======================================
Files 98 98
Lines 36450 36461 +11
=======================================
+ Hits 32071 32082 +11
Misses 4379 4379 ☔ View full report in Codecov by Sentry. |
…o JAA/is_nilpotent
| @@ -115,6 +115,8 @@ is_zero(a::QQFieldElemOrPtr) = is_zero(_num_ptr(a)) | |||
|
|
|||
| is_unit(a::QQFieldElem) = !iszero(a) | |||
|
|
|||
| is_nilpotent(a::QQFieldElem) = iszero(a) | |||
There was a problem hiding this comment.
I am not sure we really need or want these methods here -- I would expect this to be handled by a generic method in AA, something like this:
function is_nilpotent(a::T) where {T <: RingElem}
is_domain_type(parent_type(T)) && return izero(a)
throw(NotImplementedError(:is_nilpotent, a))
end
In general I think it would be better to first have the generic code in AA ready. BTW you mentioned again during the call today the problem of dealing with infinite series -- so I'll also mention again that this is not necessary for now -- the above method takes care of it well enough for now.
So a first PR just adding e.g. that method and perhaps a few tests would already be a good start. Adding is_unit methods can then come in one more further PRs. And then once AA is ready and released we can add missing or optimized bits in Nemo.
src/flint/fmpz_mod.jl
Outdated
| @@ -20,6 +20,8 @@ base_ring(a::ZZModRing) = ZZ | |||
|
|
|||
| parent(a::ZZModRingElem) = a.parent | |||
|
|
|||
| is_domain_type(::Type{ZZModRingElem}) = false | |||
There was a problem hiding this comment.
This is not necessary as we have this in AA:
# Type can only represent elements of domains
# false unless explicitly specified
is_domain_type(R::Type{T}) where T <: RingElem = false
src/HeckeMoreStuff.jl
Outdated
| g = gcd(r, m) | ||
| (g == m) && return true | ||
| (g == 1) && return false | ||
| m /= g |
There was a problem hiding this comment.
| m /= g | |
| m = divexact(m, g) |
Since e.g. / is doing the wrong thing if T === Int:
julia> Int(2)/Int(3)
0.6666666666666666
There was a problem hiding this comment.
Bother! I was hoping to do "in place" arithmetic with m /= g but I suspect that m = divexact(m,g) is not in place. Too bad, I guess. I'll check what the time penalty is...
There was a problem hiding this comment.
m /= g is just syntactic sugar for m = m / g and does not work in place anyway.
We have divexact! for a few types to support in-place, but AFAIK this is still mostly in Nemo and and not yet generically implemented in AA. That could be subjective of a future PR. For now just use divexact ?
There was a problem hiding this comment.
Question: in AbstractAlgebra/src/Residue.jl near line 73 there is the function definition
lift(a::ResElem{Int}) = BigInt(data(a))
Why is the result cast to a BigInt? I had expected the result to be of type Int or maybe UInt; and if the result is to be cast to a "big integer" then wouldn't ZZRingElem be more natural (but then it seems that ZZRingElem is not visible inside AbstractAlgebra... sigh!)
There was a problem hiding this comment.
divexact was faster -- good! I shall replace the call to lift by a call to data so that the code actually does what the reader might reasonably expect. I shall also add an overflow test.
There was a problem hiding this comment.
I've pushed all the changes... waiting for GitHub to find my mistakes...
Usefully faster implementation of
is_nilpotent(::ResElem), but it is longer...