"Inplace" iterators over the coefficients, monomials, terms and exponent vectors of a multivariate polynomial #2196
Conversation
| if !isassigned(m.coeffs, 1) || !is_one(m.coeffs[1]) | ||
| m.coeffs[1] = one(base_ring(x)) | ||
| end |
There was a problem hiding this comment.
This is a bit ugly, but it gets the monomials iterator to constant allocations. Otherwise it would allocate a new 1 all the time. Is this too much of a corner case?
joschmitt
force-pushed
the
js/inplace
branch
2 times, most recently
from
4f33ce7 to
7275f84
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2196 +/- ##
==========================================
- Coverage 88.01% 88.00% -0.01%
==========================================
Files 127 127
Lines 31765 31798 +33
==========================================
+ Hits 27957 27984 +27
- Misses 3808 3814 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
src/generic/GenericTypes.jl
Outdated
| # S may be the type of anything that can store an exponent vector, for example | ||
| # Vector{Int}, ZZMatrix, ... | ||
| struct MPolyExponentVectors{T <: AbstractAlgebra.RingElem, S} | ||
| poly::T | ||
| inplace::Bool | ||
| temp::S # only used if inplace == true | ||
| end |
There was a problem hiding this comment.
The exponent_vectors iterator can now in principle take anything as type for the exponents, as long as there is a exponent_vector!(::T, ...) method for that type. Now one can do exponent_vectors(Vector{ZZRingElem}, ::QQMPolyRingElem) and it would for example allow to add a variant exponent_vectors(ZZMatrix, ...) in Nemo like mentioned in oscar-system/Oscar.jl#5483 (comment) .
|
Thank you very much for working on this. It seems fine to me. But I'd like triage to have a look at it before merging. |
joschmitt
changed the title
joschmitt
added
the
release notes: use title
|
IMO this should also have the abstract fallbacks, see Nemocas/Nemo.jl#2161 (comment) |
Also done. |
lgoettgens
left a comment
lgoettgens
left a comment
There was a problem hiding this comment.
some more minor comments. I didn't really think about exponent vectors yet, mostly about the iterators that return some ring elem of some kind
| function coefficients(a::MPolyRingElem{T}) where T <: RingElement | ||
| return Generic.MPolyCoeffs(a) | ||
| function coefficients(a::MPolyRingElem{T}; inplace::Bool = false) where T <: RingElement | ||
| return Generic.MPolyCoeffs(a, inplace=inplace) |
There was a problem hiding this comment.
| return Generic.MPolyCoeffs(a, inplace=inplace) | |
| return Generic.MPolyCoeffs(a; inplace) |
IMO a bit neater, but I don't really care
There was a problem hiding this comment.
I never really got the hang of this syntax, so if you don't care, I'll prefer my version.
lgoettgens
left a comment
lgoettgens
left a comment
There was a problem hiding this comment.
Looks great! Thanks for working on this!
3 participants
Add an
inplaceoption to the iterators of exponents, coefficients, terms and monomials of a multivariate polynomial.This is still a draft. Is this the "interface" we want?
Regarding the new functions
coeff!,term!andexponent_vector!: Should they be exported (I did it for now becausemonomial!is exported as well)? Do I need to implement an "abstract fall-back" insrc/MPoly.jl?Nemocas/Nemo.jl#2161 implements the (few) necessary functions for
QQMPolyRingElem. Using those, I get the following timings in oscar-system/Oscar.jl#5483 :So that moves in the right direction.