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 oscar-system/Singular.jl#908, that is:

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

See also #1853, #1854, #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 24ffa6d from PR #285 (and now I am teaching so I am not going to follow up soon)

[codecov]

Codecov Report

❌ Patch coverage is 75.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 88.07%. Comparing base (88c673c) to head (fc670f5).

Files with missing lines	Patch %	Lines
src/NCRings.jl	75.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2212      +/-   ##
==========================================
- Coverage   88.08%   88.07%   -0.02%     
==========================================
  Files         126      126              
  Lines       31729    31729              
==========================================
- Hits        27949    27944       -5     
- Misses       3780     3785       +5     

☔ 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]

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.

[fingolfin]

Resolve the merge conflicts and added some more matrix tests. This is good to go from my viewpoint.