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

Ensure MatElem subtypes T have T(R::Ring, r::Int, c::Int) constructor #1791

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

fingolfin
Copy link
Member

This would be a step towards resurrecting Nemocas/AbstractAlgebra.jl#558 i.e. to reduce the code MatElem subtypes must implement.

In particular the new constructors would by default produce uninitialized matrices and then other constructors like zero_matrix, identity_matrix or ones_matrix could be built atop. (In the first mentioned PR there is also a type trait is_zero_initialized(T::Type{<:MatrixElem}) which can be used to "optimize" e.g. zero_matrix or identity_matrix for matrix types where the new matrices are zero matrices anyway).

I am marking this as a draft because there are some call sites that could be adjusted, but before putting into any more effort into this, I wanted to check if this is a direction we want to go into to begin with.

@@ -11,8 +11,7 @@
###############################################################################

function similar(::AcbMatrix, R::AcbField, r::Int, c::Int)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the future this and its many siblings could be replaced by a single default method

similar(::T, R::AcbField, r::Int, c::Int) where T <: MatElem = T(R, r, c)

@@ -747,8 +746,7 @@ end
###############################################################################

function (x::AcbMatSpace)()
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently we already have a default method for this which is

function (a::MatSpace{T})() where {T <: NCRingElement}
   return zero_matrix(base_ring(a), nrows(a), ncols(a))::dense_matrix_type(T)
end

but that means we implicitly expect zero_matrix to be implemented... With the new system both can be implemented generically.

Copy link

codecov bot commented Jun 12, 2024

Codecov Report

Attention: Patch coverage is 83.33333% with 10 lines in your changes missing coverage. Please review.

Project coverage is 87.28%. Comparing base (ef20920) to head (dbd37b9).

Files with missing lines Patch % Lines
src/flint/FlintTypes.jl 71.42% 8 Missing ⚠️
src/arb/ArbTypes.jl 80.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1791      +/-   ##
==========================================
- Coverage   87.32%   87.28%   -0.05%     
==========================================
  Files          97       97              
  Lines       35658    35666       +8     
==========================================
- Hits        31139    31130       -9     
- Misses       4519     4536      +17     

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

# used by windows, not finalised!!
# MatElem interface
function FqMatrix(R::FqField, r::Int, c::Int)
return FqMatrix(r, c, R)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For FqMatrix we already have FqMatrix(r, c, R) but I don't see a downside with having both that and FqMatrix(R, r, c). Down the road we could think about abolishing the former in a breaking release, but there is no strong need either.

(One could also argue that we should just use the (r, c, R) order everywhere. Would be also OK to me. I used the former because (a) the old PR used it and (b) it matches the order in matrix(R, r, c, ...), zero_matrix, and some other places

I think @thofma added the FqMatrix constructor here, so perhaps he had a deeper reason for the different order that we should heed?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason is that I copy & pasted it from some other matrix constructor in Nemo. There is no deeper reason. It is not part of the API, so can be changed/augmented/removed.

@thofma
Copy link
Member

thofma commented Jun 13, 2024

Before looking at this more closely, I wanted to ask what the overall aim of this is? If it is just about reducing the necessary functions in the matrix API, we could reduce everything to zero_matrix and be done with it. I am not sure that introducing more API via is_uninitialized... etc really reduces the API?

If we want to go down this route, maybe we can reuse the undef thing from the Array construction? I like that it forces one to be aware that the entries might be undefined/gargabe:

julia> Matrix{Int}(2, 2)
ERROR: MethodError: no method matching Matrix{Int64}(::Int64, ::Int64)
[...]

julia> Matrix{Int}(undef, 2, 2)
2×2 Matrix{Int64}:
 4688102416  19
 4854862112   0

@fingolfin
Copy link
Member Author

This goes back to this comment by @fieker which stated:

What is the natrual cosntructor for an not intialized matrix? A coompon
use case is allocate a matrix, fill it (ie. vcat, hcat) insisting in
having a well defined input matrix is wasteful

Actually, I understood Tommy's comment to be in the same vein, but re-reading it, I see now that it can also be interpreted as just saying "why not let zero_matrix be the central constructor" -- for some data types this is indeed "free", but not necessarily for all.

I followed the idea here that I don't want to make anything worse than it is right now.

Regarding adapting the Matrix{Int}(undef, 2, 2) syntax: could also do that, but how would you envision that to look like? ZZMatrix(ZZ, undef, 2, 2) ? Actually perhaps ZZMatrix(ZZ, 2, 2, undef) would be a bit more natural in our context? (I am totally open to either or something else)

Anyway, this kind of discussion is precisely why I did not want to complete this PR, just do enough to showcase the situation and we can hash it out.

@fieker do you know from the top of your head if we have any existing examples of matrix types that actually allow creating an "uninitialized" matrix? Of course even if there are none, that's not a good reason to not plan for them. But it would allow setting up tests and to take some timings.

@fingolfin
Copy link
Member Author

[...] what the overall aim of this is? If it is just about reducing the necessary functions in the matrix API,

In a nutshell, that's it, but the true goal then is to on the one hand make it easier to create further confirming matrix implementation, and on the other hand to remove various minor discrepancies between our existing implementations and to make it easier to implement "optimal" code in other situations (that's not yet in here of course).

@fingolfin
Copy link
Member Author

Resurrecting this. There seem we to be new use cases, e.g. #1826 and in any case I don't see downsides to having this in, as this is all internal API. It would at least allow further experiments towards resurrecting Nemocas/AbstractAlgebra.jl#558 as stated

I am also still open to changing this to e.g. ZZMatrix(ZZ, undef, 2, 2) or ZZMatrix(ZZ, 2, 2, undef) or something else, perhaps @thofma could chime in to say if he prefers either of those (and which) over ZZMatrix(ZZ, 2, 2) for producing an "uninitialized" matrix (here ZZ and ZZMatrix are placeholders for any Nemo matrix types)

@lgoettgens
Copy link
Collaborator

I am also still open to changing this to e.g. ZZMatrix(ZZ, undef, 2, 2) or ZZMatrix(ZZ, 2, 2, undef) or something else, perhaps @thofma could chime in to say if he prefers either of those (and which) over ZZMatrix(ZZ, 2, 2) for producing an "uninitialized" matrix (here ZZ and ZZMatrix are placeholders for any Nemo matrix types)

I think putting undef in the end would more consistent with matrix(ZZ, 2, 2, [some values]), but no strong preference

@thofma
Copy link
Member

thofma commented Oct 9, 2024

I agree with @lgoettgens.

@fingolfin
Copy link
Member Author

I also prefer undef at the end and will change this PR accordingly when I find the time

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.

3 participants