Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor MatrixSpace implementation #1433

Closed
wants to merge 2 commits into from

Conversation

fingolfin
Copy link
Member

@fingolfin fingolfin commented Mar 27, 2023

This is work in progress and a proof of concept. Assuming all passes here, too, the next step would be to replace the AbstractAlgebra.Generic.MatSpace implementation with the GenericMatSpace type defined in this PR, and then hopefully we can kill all other MatSpace subtypes.

This also removes most of the performance overhead involved in using matrix spaces as matrix factories. Indeed, consider these test functions:

f1(n) = zero_matrix(GF(2), n, n)
f2(n) = matrix_space(GF(2), n, n)()

Timings before this PR:

julia> @btime f1(10); @btime f2(10);
  115.881 ns (3 allocations: 992 bytes)
  194.739 ns (3 allocations: 992 bytes)

Timings with this PR:

julia> @btime f1(10); @btime f2(10);
  119.678 ns (3 allocations: 992 bytes)
  122.296 ns (3 allocations: 992 bytes)

@codecov
Copy link

codecov bot commented Mar 28, 2023

Codecov Report

Merging #1433 (812a0c5) into master (7aee528) will increase coverage by 0.13%.
The diff coverage is 100.00%.

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

@@            Coverage Diff             @@
##           master    #1433      +/-   ##
==========================================
+ Coverage   88.67%   88.80%   +0.13%     
==========================================
  Files          88       89       +1     
  Lines       34237    34147      -90     
==========================================
- Hits        30358    30323      -35     
+ Misses       3879     3824      -55     
Impacted Files Coverage Δ
src/Nemo.jl 29.88% <ø> (+2.93%) ⬆️
src/arb/ArbTypes.jl 59.05% <ø> (+2.42%) ⬆️
src/flint/FlintTypes.jl 94.78% <ø> (-0.20%) ⬇️
src/flint/fmpz_mod_mat.jl 87.64% <ø> (-1.18%) ⬇️
src/flint/gfp_mat.jl 93.26% <ø> (-0.68%) ⬇️
src/flint/nmod_mat.jl 92.64% <ø> (-0.43%) ⬇️
src/MatrixSpace.jl 100.00% <100.00%> (ø)
src/arb/ComplexMat.jl 77.27% <100.00%> (+0.02%) ⬆️
src/arb/RealMat.jl 80.12% <100.00%> (+0.06%) ⬆️
src/arb/acb_mat.jl 78.14% <100.00%> (+0.02%) ⬆️
... and 7 more

... and 34 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@fingolfin
Copy link
Member Author

So as a proof of concept, this seems to work, all tests including OscarCI.

Next step: adapting AbstractAlgebra.Generic.MatSpace.

@fingolfin
Copy link
Member Author

I am closing this for now to avoid interference with Nemocas/AbstractAlgebra.jl#1318 -- I do not want to that PR against this one here, but rather that PR should work on its own. Once it does and is merged, I'll either reopen this PR or make a new one.

@fingolfin fingolfin closed this Apr 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant