Some matrix optimizations

[joschmitt]

Including and extending the work of @fieker.

I add tests once I see the CI result as usual. (Which needs an AbstractAlgebra update; I forgot.)

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.77%. Comparing base (5fac404) to head (ad676fe).

❗ Current head ad676fe differs from pull request most recent head aab6f83. Consider uploading reports for the commit aab6f83 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1712      +/-   ##
==========================================
+ Coverage   84.68%   84.77%   +0.08%     
==========================================
  Files          94       94              
  Lines       37139    37296     +157     
==========================================
+ Hits        31451    31616     +165     
+ Misses       5688     5680       -8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[joschmitt]

Should this function be added for all the different flint matrix types? And is there a better way to do it?
With the pointer it needs 1 allocation (I assume for one(F)) in comparison to 3 allocations for the naive a[i, j] += 1.

[thofma]

There is fq_default_sub_one, but no fq_default_add_one. So you could do a neg!(sub_one!(neg!(x))) to do it allocation free. Not sure if this is a good idea.

Any ideas @fieker?

[fieker]

On Thu, Apr 11, 2024 at 07:12:36AM -0700, Johannes Schmitt wrote: @joschmitt commented on this pull request. > @@ -78,6 +78,25 @@ end setindex!(a::FqMatrix, u::Integer, i::Int, j::Int) = setindex!(a, base_ring(a)(u), i, j) +function setindex!(a::FqMatrix, b::FqMatrix, r::UnitRange{Int64}, c::UnitRange{Int64}) + _checkbounds(a, r, c) + size(b) == (length(r), length(c)) || throw(DimensionMismatch("tried to assign a $(size(b, 1))x$(size(b, 2)) matrix to a $(length(r))x$(length(c)) destination")) + A = view(a, r, c) + ccall((:fq_default_mat_set, libflint), Nothing, + (Ref{FqMatrix}, Ref{FqMatrix}, Ref{FqField}), A, b, base_ring(a)) +end + +function Generic.add_one!(a::FqMatrix, i::Int, j::Int) Should this function be added for all the different flint matrix types? And is there a better way to do it? With the pointer it needs 1 allocation (I assume for `one(F)`) in comparison to 3 allocations for the naive `a[i, j] += 1`.

add_one is intended to not do any alloc. If it does, then my code is stupid. Almost all flint rings allow add_si or similar to not create the ring elem first. Asymptotically, it should be for all types, however, for nmod disguised as Fq the generic fallback is particularly bad. Furthermore, the only code calling it is from the inj/proj mat stuff... I think

…

-- Reply to this email directly or view it on GitHub: #1712 (review) You are receiving this because you were mentioned. Message ID: ***@***.***>

[thofma]

fq_default does not have the add_si version.

[joschmitt]

I see what I can do.

[fieker]

On Thu, Apr 11, 2024 at 07:40:35AM -0700, Tommy Hofmann wrote: `fq_default` does not have the `add_si` version.

but the nmod does I think. IN particular since 1 is a valid nmod.

…

-- Reply to this email directly or view it on GitHub: #1712 (comment) You are receiving this because you were mentioned. Message ID: ***@***.***>

[joschmitt]

I added add_one! for the flint matrix types. It is allocation free except for fq_mat and fq_nmod_mat (and the corresponding cases in fq_default_mat). For these, neg(sub_one(neg(...))) is better than add(..., one), but still needs one allocation.

[joschmitt]

I corrected the fmpz_mod cases as discussed, so this is good to go now.

[thofma]

Thanks