-
Notifications
You must be signed in to change notification settings - Fork 57
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 #1738
Conversation
4c13b80
to
c35940f
Compare
c35940f
to
78924b5
Compare
The tests now no longer need to be removed due to #1748 |
3e326b1
to
185b43a
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1738 +/- ##
==========================================
- Coverage 84.91% 84.84% -0.08%
==========================================
Files 95 95
Lines 37281 36957 -324
==========================================
- Hits 31657 31355 -302
+ Misses 5624 5602 -22 ☔ View full report in Codecov by Sentry. |
185b43a
to
1ef145b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All changes look good. There are some more matrix constructors that IMO should work generically with the new AA functionality and could thus be removed. A few got a comment on them.
But this can surely be postponed to a future PR
end | ||
return M | ||
end | ||
|
||
function (a::FpMatrixSpace)(b::FpFieldElem) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would expect this one to work generically
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case that might actually be true -- but still, I'd really prefer to move forward now, and if you or someone else would like to remove further methods in a follow-up, be my guest...
end | ||
return M | ||
end | ||
|
||
function (a::fqPolyRepMatrixSpace)(b::fqPolyRepFieldElem) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would expect this one and the ones below to work generically
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While there is a generic method, this method is actually optimized (though I don't know how effective the optimization is). I am reluctant to remove it as part of this PR. If someone would like to thoroughly test it and compare the performance (and allocations) of this code to the generic one, and then possibly (depending on the outcome) remove this here: fine by me. But I didn't want this PR to get bogged down by this kind of work (it's already been in the works for over a year at this point!)
@@ -1084,14 +1070,3 @@ function eigenvalues(A::ComplexMat) | |||
e, _ = _eig_multiple(A) | |||
return [ x[1] for x in e ] | |||
end | |||
|
|||
############################################################################### |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In terms of "things we can probably remove now: also the code in src/arb
has some methods like e.g. this one:
function (x::ComplexMatSpace)()
z = ComplexMat(nrows(x), ncols(x))
return z
end
which could go; similar code is in RealMat.jl
/acb_mat.jl
/arb_mat.jl
.
Oh and also this could (mostly) go:
for T in [Integer, ZZRingElem, QQFieldElem, Float64, BigFloat, RealFieldElem, ComplexFieldElem, String]
@eval begin
function (x::ComplexMatSpace)(y::$T)
z = x()
for i in 1:nrows(z)
for j = 1:ncols(z)
if i != j
z[i, j] = zero(base_ring(x))
else
z[i, j] = y
end
end
end
return z
end
end
end
The exception would be the variant taking a String
. That could be implemented like this (which I am guessing would also be more efficient -- only parse the string once):
(x::ComplexMatSpace)(y::AbstractString) = x(ComplexFieldElem(y))
ncols::Int | ||
|
||
function RealMatSpace(R::RealField, r::Int, c::Int) | ||
(r < 0 || c < 0) && throw(_err_dim_negative) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, and we can now delete _err_dim_negative
again as there are no references to it left in Nemo.
Resolves #1379
Resolves #1317
Counterpart to Nemocas/AbstractAlgebra.jl#1318 (which this requires)
Probably won't work right now, I just rebased this code I started over a year ago and then abandoned :-/