Make promote a no-op if inputs have same type

[lgoettgens]

to satisfy the assumption noted in

AbstractAlgebra.jl/src/NCRings.jl

Line 107 in 67299a7

# we assume that promotion returns identical operands if no proper

, namely that "we assume that promotion returns identical operands if no proper promotion can be performed".

This should also change the silent stackoverflow from oscar-system/Oscar.jl#5975 into a proper NotImplementedError. To verify that this does not break anything, I would like to wait with merging until oscar-system/Oscar.jl#5982 and oscar-system/Oscar.jl#5985 are merged.

[codecov]

Codecov Report

❌ Patch coverage is 62.50000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.11%. Comparing base (ce7ecbb) to head (67299a7).
⚠️ Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
src/Matrix.jl	66.66%	2 Missing ⚠️
src/NCRings.jl	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2405      +/-   ##
==========================================
- Coverage   88.13%   88.11%   -0.02%     
==========================================
  Files         130      130              
  Lines       32948    32956       +8     
==========================================
+ Hits        29038    29040       +2     
- Misses       3910     3916       +6     

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[thofma]

In theory this will not work for finite fields, since finite fields allow promotion between them and all have the same type:

julia> F = GF(2);

julia> FF = GF(4);

julia> FF(F(1))
1

On the other hand, the promotion system is already broken for this example, so we might as well break it some more.

[fingolfin]

First off: if this fixes a concrete issue and both @lgoettgens and @thofma are happy with it, go ahead, I don't want to block progress.

That said, I have some questions (which could also be turned into an issue for later).

What if S === T but typeof(x) !== typeof(y)? I.e. different matrix types over the same ring type? We normally don't have that, unless something went wrong, so perhaps it is OK to ignore; it just seems odd.

More importantly, as @thofma already pointed out: what if S === T but base_ring(x) != base_ring(y) ?

Do we have a way to detect if the "base rings have a common promotion"?

One could of course do something stupid like this

Suggested change

	if S === T
	return x, y
	end
	if S === T
	if base_ring(x) != base_ring(y)
	R = parent(one(base_ring(x)) + one(base_ring(y))) # common base ring
	base_ring(x) === R && return x, change_base_ring(R, y)
	base_ring(y) === R && return change_base_ring(R, x), y
	return change_base_ring(R, x), change_base_ring(R, y)
	end
	return x, y
	end

Of course the way R is computed is a nasty hack. To do better I guess we would need a helper that takes two rings and returns their "promotion" -- or maybe this already exists?

I imagine that such a helper would always return one of its two argument, or else indicate a problem (by raising an error or returning nothing or so). It would have to be written carefully to match the actual promotion behaviour.

(BTW, it seems change_base_ring(R, x) always creates a new object, even if R is already the base ring of (the parent of) x. Is this intentional? Then it should be documented. If it is not, perhaps we should change that?)

[thofma]

There is a problem, since the addition will throw in general (l. 1183), e.g. for finite fields.

I think we should leave possible improvements to a proper value based promotion system (I have an idea for it, but did not find the time yet to implement it). The changes are of a larger scale, since currently we do not keep track of enough information.