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

change_base_ring desired behaviour #490

Closed
wbhart opened this issue Oct 10, 2019 · 5 comments
Closed

change_base_ring desired behaviour #490

wbhart opened this issue Oct 10, 2019 · 5 comments

Comments

@wbhart
Copy link
Contributor

wbhart commented Oct 10, 2019

@thofma and I had a (very) long discussion about what behaviour we ultimately want.

In his case, he wants change_base_ring(ZZ, M) in Nemo, where M is a Generic.MatSpaceElem{nf_elem} to return an fmpz_mat. Reason: both fmpz_mat and Generic.MatSpaceElem{nf_elem} are matrices that Nemo would create.

In my case, I want change_base_ring(Nemo.ZZ, p) in Singular, where p is a Singular spoly{fmpq} to return an spoly{fmpz}. Reason: both polynomials are what Singular would create over a Nemo coefficient ring.

So we don't disagree on what we want, it's just not implemented that way at present (and probably wasn't implemented correctly before the new similar functionality, either).

The following simple 7 steps will lead to balance being restored in the force:

  1. [Removed] see the issue for similar/map which is now essentially orthogonal to the change_base_ring issue.

  2. Create abstract types NemoMatrixElem{T} <: AbstractAlgebra.MatrixElem{T}, NemoPolyElem{T} <: AbstractAlgebra.PolyElem{T}, NemoMPolyElem{T} <: AbstractAlgebra.MPolyElem{T} and make all Nemo matrix and polynomial element types belong to one of these, instead of directly to the AbstractAlgebra abstract types.

2a. Create an abstract type NemoRing <: AbstractAlgebra.Ring and make all Nemo ring element types belong to this instead of directly to the AbstractAlgebra abstract type.

  1. In AbstractAlgebra, implement generic fallback change_base_ring functions, which always creates generic Mat/Poly/MPoly's. It will have signature, e.g: change_base_ring(::Ring, ::AbstractAlgebra.MatElem).

  2. For Nemo rings with native matrix, poly or mpoly types, e.g. fmpz, implement change_base_ring(::fmpz, ::NemoMat). This should always create a Nemo matrix (fmpz_mat, in the example).

  3. For Nemo rings without native matrix, poly or mpoly types, e.g. nf_elem, implement change_base_ring(::NemoRing, Generic.MatSpaceElem{nf_elem}) which will always construct a Nemo matrix. This will ensure @thofma gets the functionality he wants.

  4. change_base_ring will NOT be implemented in terms of similar.

  5. [Removed]

I don't discuss here how we handle the Singular functionality, which allows for creating Singular polynomials over Nemo coefficient rings and vice versa. But it is clear that the strategy above is consistent across all our systems and will work there too.

@rfourquet
Copy link
Contributor

Remove change base ring functionality from similar, i.e. similar will only create a similar kind of matrix over the same base ring. This will fix the -m issue.

Can we not do that? It's not the "base ring" functionality from similar in itself which creates the problem with -m, it's the way it has been implemented, which differs from the expectation of having similar(m) create a matrix of the same type as m.

NemoMatrixElem{T} <: AbstractAlgebra.MatrixElem{T}

I guess you meant MatElem instead of MatrixElem. An alternative is to have NemoMatrixElem be a Union of the Nemo matrix types. I'm not clear in how to formulate that, but It's customary to create an abstract type to refer to a "math entity", or to a certain "behavior/attribute" of the types which inherit from it (sorry for the vagueness of this sentence), for exemple DenseArray, and which in principle is "open", in that it's expected that users/other packages will create their own types which will inherit from it. Here NemoMatrixElem is a closed set, with a fixed number of types, and which we don't want anyone else to inherit from.
As an exemple, there is Signed and Unsigned abstract types, but Base.BitInteger is a Union. Similarly, Base.IEEFloat is a Union which contains the AbstractFloat implemented in Base.

map_entries/coeffs(f, m) should dispatch to map_entries/coeffs(R, f, M) where R = parent(zero(base_ring(M))).

So, to take the example from the issue, does it mean that map(identity, m) will continue to not necessarily return a matrix of the same type as m?

@wbhart wbhart changed the title similar, map_coeffs, map_entries desired behaviour similar, map_coeffs, map_entries, change_base_ring desired behaviour Oct 11, 2019
@wbhart
Copy link
Contributor Author

wbhart commented Oct 24, 2019

Remove change base ring functionality from similar, i.e. similar will only create a similar kind of matrix over the same base ring. This will fix the -m issue.

Can we not do that? It's not the "base ring" functionality from similar in itself which creates the problem with -m, it's the way it has been implemented, which differs from the expectation of having similar(m) create a matrix of the same type as m.

I thought similar was now supposed to create an uninitialised matrix. Why should it care what the base ring is? The only reason to pass that in is if it were going to create an initialised object. It should only need the type of elements in the base ring, not the actual base ring itself.

If that's not actually possible, then we should not be using similar and should go back to how things were, i.e. requiring a AbstractAlgebra/Nemo matrix to always be constructed in an initialised state.

The fact that we could get around the problem by forcing the user to supply a base ring, does not mean we should. It's inconsistent semantics.

@rfourquet
Copy link
Contributor

I thought similar was now supposed to create an uninitialised matrix

That was the idea indeed, while zero was supposed to share the same API but creating a zero-initialized matrix instead.

Why should it care what the base ring is?

Because for example, AA matrices contain a field base_ring. After calling (the current version of) similar, I only have to bother to fill out all the matrix positions, with indexing. This is a universal API for matrices, whereas updating an internal field may not be (I may be mistaken, but I don't remenber that the base_ring internal field is public API). Moreover, certain matrices might require to know the base ring at creation time.

That said, I don't see a big problem with also having similar methods taking the type of elements and not the base ring, but it feels even more low level and I don't understand where it is needed.

If that's not actually possible, then we should not be using similar and should go back to how things were, i.e. requiring a AbstractAlgebra/Nemo matrix to always be constructed in an initialised state.

It's possible, but I don't understand you conclusion that all matrices always have to be constructed in an initialised state. It's good to usually return a matrix in an initialized state of course, but a low level tool to return an unitialized matrix has it place, for when you implement a routine which initializes all the entries of the matrix anyway.

Again, similar accepting a ring is not the cause of the bugs that we are trying to solve here.

The fact that we could get around the problem by forcing the user to supply a base ring, does not mean we should. It's inconsistent semantics.

Sorry, I don't understand this sentence, I don't remember where we forced the user to supply a base ring (in similar there is a default one, based on the input matrix).

@wbhart wbhart changed the title similar, map_coeffs, map_entries, change_base_ring desired behaviour change_base_ring desired behaviour Oct 24, 2019
@wbhart
Copy link
Contributor Author

wbhart commented Oct 24, 2019

I've now updated the steps above after a long discussion with @rfourquet

The similar/map_coeffs/map_entries functions will now be implemented using a different strategy and the change_base_ring functionality will not (cannot) depend on it. So the two issues are now orthogonal.

@wbhart
Copy link
Contributor Author

wbhart commented Dec 11, 2019

I believe we now have change_base_ring sorted out (though perhaps not the way we thought). So I will close this issue.

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

No branches or pull requests

2 participants