`Base.promote(::NCRingElem, ::MatrixElem)` does not promote to a common type

[JohnAAbbott]

In PR #2420 @lgoettgens has proposed a replacement for Base.promote(::MatrixElem, ::MatrixElem) where the idea to replace MatrixElem with MatElem and/or MatRingElem (so there are 4 cases to consider here).

What should happen in the following scenario?

julia> R = matrix_ring(ZZ, 3);

julia> S = matrix_space(ZZ, 3, 3);

julia> Base.promote(one(R), one(S))
[[[what happens here?]]]

Current code gives an error. The proposed new code wants to promote to matrix_space(matrix_ring(ZZ,3), 3,3), but then returns an "asymmetrical" result -- see the second part of the first comment in PR #2420

[JohnAAbbott]

Here is an asymmetrical result produced by the current code:

julia> R = matrix_ring(ZZ, 3);

julia> S = matrix_space(ZZ, 3, 3);

julia> Base.promote(QQ(2), one(S))
(2, [1 0 0; 0 1 0; 0 0 1])

julia> Base.promote(QQ(2), one(R))
ERROR: Cannot promote to common type

Given the documentation for Base.promote, it seems strange that the result can produce two values of different types. It is also strange that MatElem and MatRingElem behave differently: one gives an error while the other does not.

[lgoettgens]

I think the main point of this discussion should not be about MatRingElem in particular (I already added the triage label to #2420 (comment) for that particular issue).

The problem that @JohnAAbbott pointed out in #2420 (comment) is that we have a Base.promote(::NCRingElem, ::MatElem) method that does not promote both inputs to the same type, but rather the first argument to the base ring of the second.

One way to resolve this would be to rename the function for this particular case. Alternatively, we could patch the method to not only do what it currently does, but additionally change the output NCRingElem to a diagonal matrix with that element on the diagonal, so that both outputs have the same type.

[JohnAAbbott]

Promoting both values to matrices fits correctly with the documented behaviour of Base.promote BUT that is likely undesirable behaviour (unless a "clever" representation for scalar multiples of an identity matrix is available) since we risk losing the information that one value is really just a scalar.

[fingolfin]

Question by triage regarding "the problem [...] is that we have a Base.promote(::NCRingElem, ::MatElem) method that does not promote"

In how far is this a problem? Does it cause actual issues? What are they?

[lgoettgens]

Question by triage regarding "the problem [...] is that we have a Base.promote(::NCRingElem, ::MatElem) method that does not promote"

In how far is this a problem? Does it cause actual issues? What are they?

It violates the contract of Base.promote. I am not aware of any real-life issues with this, apart from human confusion. (but maybe @JohnAAbbott does)

[JohnAAbbott]

I am also unaware of specific instances where this leads to incorrect code behaviour, but, as @lgoettgens wrote, there is the problem that the implementation does not behave as documented (see Julia documentation for Base.promote) which could lead to future code problems.