AbstractAlgebra.jl icon indicating copy to clipboard operation
AbstractAlgebra.jl copied to clipboard

Memory allocation in diagonal_matrix

Open a-kulkarn opened this issue 3 years ago • 9 comments

Hello AbstractAlgebra folks,

Is this intended behaviour?

using Nemo

I = identity_matrix(QQ, 5)
I[1,1] === I[2,2] # evaluates to false

Qp = padic_field(5)
Ip = identity_matrix(Qp, 5)
Ip[1,1] === Ip[2,2] # evaluates to true

If so, I think it might be nice to make a note of this peculiarity in the documentation. In either case, I'm happy to follow this up with a PR.

a-kulkarn avatar Jan 28 '22 16:01 a-kulkarn

~I think the second one is bad.~ Alright, the decision apparently was that both are acceptable.

tthsqe12 avatar Jan 28 '22 16:01 tthsqe12

No, this is intentional, see the discussion in https://github.com/Nemocas/AbstractAlgebra.jl/pull/692

thofma avatar Jan 28 '22 16:01 thofma

It is documented as the first item here: http://nemocas.github.io/Nemo.jl/latest/developer/topics/#Aliasing-rules

thofma avatar Jan 28 '22 16:01 thofma

hmm. I see this in there:

Constructors for polynomials, series and similar ring element objects (that are not matrices) that take an array as input, must ensure that the coefficients being placed into the object do not alias, even in part

Are we even doing this?

julia> using AbstractAlgebra

julia> R, x = PolynomialRing(ZZ, "x")
(Univariate Polynomial Ring in x over Integers, x)

julia> a = ZZ(1)
1

julia> p = R([a, a, a])
x^2 + x + 1

julia> coeff(p, 0) === coeff(p, 1)
true

tthsqe12 avatar Jan 28 '22 16:01 tthsqe12

Yes, you are constructing it (you are the "Constructor") and must ensure that the coefficients do not alias each other. We are not checking this :)

thofma avatar Jan 28 '22 16:01 thofma

I think it is badly worded. I think it means that users of such constructors...

wbhart avatar Jan 28 '22 16:01 wbhart

Sure, so the documentation is meant to push the burden onto the user. But, of course the ownership problems don't end with this.

julia> R, x = PolynomialRing(ZZ, "x")
(Univariate Polynomial Ring in x over Integers, x)

julia> a = ZZ(1);

julia> array = [a, deepcopy(a), deepcopy(a)];
julia> p = R(array)
x^2 + x + 1

julia> array[2] += 1; p
x^2 + 2*x + 1

tthsqe12 avatar Jan 28 '22 19:01 tthsqe12

That is just the normal julia "no copy on construction" behavior which is unrelated to aliasing questions I believe.

I am happy to review/discuss any proposal to have a consistent and easy to use aliasing/mutation model for both matrices and polynomials or anything else. Bonus points if it can be implemented in julia.

thofma avatar Jan 29 '22 10:01 thofma

Hmm...OK. The funny thing is that the "zero" entries in the identity matrix are unaliased, since it is created using zero_matrix.

The "Julian" solution might be to have a IdentityMatrix{T}, ZeroMatrix{T} <: MatElem type that implements the matrix interface, and for Nemo types to do their own thing with the identity_matrix, zero_matrix constructors, which it seems like they do anyways. This is yet-another-type-implementation, but I thought I would mention the idea.

Alternatively, one can introduce a matrix_malloc method. I'm not sure how often identity_matrix and zero_matrix are used as constructors, but it seems as though there is some active danger in doing so. Don't know if Tommy's comment from #692 is still relevant here.

The function works as long as you know that in your output matrix, none of the entries alias each other. At the moment the output of zero_matrix and identity_matrix satisfy this. We usually use those as the first argument to !-functions. But of course this works because of an implementation detail.

I am fully behind Nemocas/Nemo.jl#278, but maybe we need some more unsafe functions for matrices? Like add!! or something like that, where it is assumed that the entries of the output do not alias each other. I am not sure, but from our experience in the last 5 years I can tell that working with matrices is quite inefficient if we don't allow those entry-mutating functions.

a-kulkarn avatar Jan 29 '22 17:01 a-kulkarn