Restrict generic `==(x::NCRingElem, y::NCRingElem)` fallback

[fingolfin]

So far this method default to returning false if it failed to promote the arguments into a common ring. That lead to situations were comparisons between unrelated rings seemed "possible" but returned surprising results.

Motivated by https://github.com/oscar-system/Singular.jl/issues/908, that is:

julia> Singular.QQ(5) == Nemo.QQ(5)
false

See also https://github.com/Nemocas/AbstractAlgebra.jl/pull/1853, https://github.com/Nemocas/AbstractAlgebra.jl/pull/1854, https://github.com/Nemocas/AbstractAlgebra.jl/pull/1873 for related unmerged PRs ...

Let's see if this one is less problematic/painful/controversial/... sigh

CC @lgoettgens @thofma @fieker

[thofma]

Sorry, I don't fully understand what the additional branch is supposed to do. Is there something missing at the moment?

[fieker]

I think it should be triaged, and documented: what is OUR intended behaviour for == between arbitrary pairs? The default of false is due to julia - and has bitten be a couple of times.

Independently, we probably should add === as a first test?

[fingolfin]

@thofma yes, sorry, there was supposed to be another throw there. I've now re-arranged it to throw in a single place.

But the real change was that return false was replaced by throw (the problem of a possible infinite recursion existed before but in a sense was "fine" in that it already resulted in an error, just not a helpful one).

[fingolfin]

Ah of course this breaks a bunch of tests which were added by @thofma in 2019 in commit 24ffa6d1ebf1fcb15797b85c8e48ff7ab06feb3e from PR #285 (and now I am teaching so I am not going to follow up soon)

[codecov[bot]]

Codecov Report

:x: Patch coverage is 75.00000% with 1 line in your changes missing coverage. Please review. :white_check_mark: Project coverage is 87.98%. Comparing base (995a497) to head (700675b).

Files with missing lines	Patch %	Lines
src/NCRings.jl	75.00%	1 Missing :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2212      +/-   ##
==========================================
- Coverage   87.99%   87.98%   -0.02%     
==========================================
  Files         127      127              
  Lines       31769    31769              
==========================================
- Hits        27956    27951       -5     
- Misses       3813     3818       +5     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:

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

[thofma]

I like the tests, since this application appears in the "real world".

[fingolfin]

We discussed this during triage, and my understanding is that in the end we agreed (even @thofma) that we could try to go on with this.

This example from PR #285 still works even with this PR here:

julia> a = zero_matrix(ZZ, 2, 3)
[0 0 0]
[0 0 0]

julia> b = zero_matrix(ZZ, 3, 3)
[0 0 0]
[0 0 0]
[0 0 0]

julia> a in [b, a]

The relevant test should be re-enabled.

(@thofma or anyone else will correct me if I misrepresented anything unwittingly)

[thofma]

Looks good. Also

julia> a = zero_matrix(QQ, 2, 3);

julia> b = zero_matrix(ZZ, 3, 3);

julia> a in [b, a]
true

still works.