Add many `@req !is_trivial` checks

[lgoettgens]

Resolves https://github.com/Nemocas/AbstractAlgebra.jl/issues/1856.

List of split of changes:

• https://github.com/Nemocas/AbstractAlgebra.jl/pull/1858
• https://github.com/Nemocas/AbstractAlgebra.jl/pull/1859
• https://github.com/Nemocas/AbstractAlgebra.jl/pull/1860
• https://github.com/Nemocas/AbstractAlgebra.jl/pull/1861

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 90.90909% with 1 line in your changes missing coverage. Please review.

Project coverage is 88.27%. Comparing base (31f0414) to head (b274c89). Report is 3 commits behind head on master.

Files with missing lines	Patch %	Lines
src/generic/Misc/Localization.jl	0.00%	1 Missing :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1857      +/-   ##
==========================================
+ Coverage   88.17%   88.27%   +0.10%     
==========================================
  Files         120      120              
  Lines       30296    30997     +701     
==========================================
+ Hits        26712    27364     +652     
- Misses       3584     3633      +49     

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

---

🚨 Try these New Features:

• Flaky Tests Detection - Detect and resolve failed and flaky tests

[fingolfin]

Thank you!

This triggers a bunch of errors

1. In Oscar, it seems MPolyQuoRing (and maybe others) doesn't implement characteristic
2. Hecke has a bunch of failures, e.g. this one:

Error in testset LocalField/neq.jl:
Error During Test at /home/runner/work/AbstractAlgebra.jl/AbstractAlgebra.jl/oscar-dev/Hecke/test/LocalField/neq.jl:33
  Got exception outside of a @test
  ArgumentError: The zero is currently not supported as a coefficient ring.
  Stacktrace:
    [1] macro expansion
      @ ~/work/AbstractAlgebra.jl/AbstractAlgebra.jl/src/Assertions.jl:599 [inlined]
    [2] #polynomial_ring_only#131
      @ ~/work/AbstractAlgebra.jl/AbstractAlgebra.jl/src/NCPoly.jl:768 [inlined]
    [3] #polynomial_ring#130
      @ ~/work/AbstractAlgebra.jl/AbstractAlgebra.jl/src/NCPoly.jl:757 [inlined]
    [4] _minmod_easy_pp(a::ZZRingElem, b::AbsSimpleNumFieldOrderElem)
      @ Hecke ~/work/AbstractAlgebra.jl/AbstractAlgebra.jl/oscar-dev/Hecke/src/NumFieldOrd/NfOrd/Ideal/Ideal.jl:1060
    [5] _minmod_comp_pp(a::ZZRingElem, b::AbsSimpleNumFieldOrderElem)
      @ Hecke ~/work/AbstractAlgebra.jl/AbstractAlgebra.jl/oscar-dev/Hecke/src/NumFieldOrd/NfOrd/Ideal/Ideal.jl:1106
    [6] _minmod(a::ZZRingElem, b::AbsSimpleNumFieldOrderElem)
      @ Hecke ~/work/AbstractAlgebra.jl/AbstractAlgebra.jl/oscar-dev/Hecke/src/NumFieldOrd/NfOrd/Ideal/Ideal.jl:1090
    [7] assure_has_minimum(A::AbsSimpleNumFieldOrderIdeal)
      @ Hecke ~/work/AbstractAlgebra.jl/AbstractAlgebra.jl/oscar-dev/Hecke/src/NumFieldOrd/NfOrd/Ideal/Ideal.jl:628
    [8] #minimum#2028
      @ ~/work/AbstractAlgebra.jl/AbstractAlgebra.jl/oscar-dev/Hecke/src/NumFieldOrd/NfOrd/Ideal/Ideal.jl:587 [inlined]
    [9] +(x::AbsSimpleNumFieldOrderIdeal, y::AbsSimpleNumFieldOrderIdeal)
      @ Hecke ~/work/AbstractAlgebra.jl/AbstractAlgebra.jl/oscar-dev/Hecke/src/NumFieldOrd/NfOrd/Ideal/Arithmetic.jl:144
   [10] macro expansion
      @ ~/work/AbstractAlgebra.jl/AbstractAlgebra.jl/src/Assertions.jl:545 [inlined]
   [11] _decomposition(O::AbsSimpleNumFieldOrder, I::AbsSimpleNumFieldOrderIdeal, Ip::AbsSimpleNumFieldOrderIdeal, T::AbsSimpleNumFieldOrderIdeal, p::Int64)
      @ Hecke ~/work/AbstractAlgebra.jl/AbstractAlgebra.jl/oscar-dev/Hecke/src/NumFieldOrd/NfOrd/MaxOrd/Polygons.jl:796
   [12] macro expansion
      @ ~/work/AbstractAlgebra.jl/AbstractAlgebra.jl/src/Assertions.jl:267 [inlined]
   [13] prime_decomposition_polygons(O::AbsSimpleNumFieldOrder, p::Int64, degree_limit::Int64, lower_limit::Int64)
      @ Hecke ~/work/AbstractAlgebra.jl/AbstractAlgebra.jl/oscar-dev/Hecke/src/NumFieldOrd/NfOrd/MaxOrd/Polygons.jl:1039
   [14] prime_decomposition(O::AbsSimpleNumFieldOrder, p::Int64, degree_limit::Int64, lower_limit::Int64; cached::Bool)
      @ Hecke ~/work/AbstractAlgebra.jl/AbstractAlgebra.jl/oscar-dev/Hecke/src/NumFieldOrd/NfOrd/Ideal/Prime.jl:263
   [15] prime_decomposition (repeats 2 times)
      @ ~/work/AbstractAlgebra.jl/AbstractAlgebra.jl/oscar-dev/Hecke/src/NumFieldOrd/NfOrd/Ideal/Prime.jl:241 [inlined]
   [16] macro expansion
      @ ~/work/AbstractAlgebra.jl/AbstractAlgebra.jl/oscar-dev/Hecke/test/LocalField/neq.jl:36 [inlined]

[fingolfin]

Since this causes failures, perhaps we can introduce this incrementally: separate PRs for the series rings (which are not much used so hopefully won't break any upstream tests), matrix rings (heavily used but perhaps still will go through), universal polynomial rings (should go through) and finally polynomial rings (causing problems)

[lgoettgens]

The problem with MPolyQuoRing is that it may indeed be trivial. But we need a groebner basis computation to decide that

[thofma]

Is there a reason that residue rings, matrices and fraction fields are also restricted? I was mainly thinking about polynomials and series, because the code there is wrong.

[lgoettgens]

Is there a reason that residue rings, matrices and fraction fields are also restricted? I was mainly thinking about polynomials and series, because the code there is wrong.

I put one in all constructions that take a ring. If you verified that the code for these other types isn't wrong, I am happy to take it out again

[thofma]

It is correct for matrices. For fraction fields it gives errors in some situations, but no wrong results.

[lgoettgens]

Since this causes failures, perhaps we can introduce this incrementally: separate PRs for the series rings (which are not much used so hopefully won't break any upstream tests), matrix rings (heavily used but perhaps still will go through), universal polynomial rings (should go through) and finally polynomial rings (causing problems)

I'll split this PR up into multiple ones.

[lgoettgens]

Matrices are removed from this again. I lost a bit tracked if something else (apart from Poly and MPoly) remains here once all of the split off things are merged. So let's just wait for a rebase.

[fingolfin]

OscarCI and HeckeCI failures: for Oscar, see https://github.com/oscar-system/Oscar.jl/issues/4239

For Hecke, getting several errors:

Error in testset AlgAssAbsOrd/Conjugacy/Husert.jl:
Error During Test at /home/runner/work/AbstractAlgebra.jl/AbstractAlgebra.jl/oscar-dev/Hecke/test/AlgAssAbsOrd/Conjugacy/Husert.jl:1
  Got exception outside of a @test
  ArgumentError: Zero rings are currently not supported as coefficient ring.

and

Error During Test at /home/runner/work/AbstractAlgebra.jl/AbstractAlgebra.jl/oscar-dev/Hecke/test/QuadForm/Herm/Spaces.jl:130
  Test threw exception
  Expression: is_isometric(V1, V2)
  ArgumentError: Zero rings are currently not supported as coefficient ring.

[lgoettgens]

Even in the case that we fix all of the downstream issues, this needs to get released in a breaking release, to not accidentally breaking Oscar 1.2

[thofma]

More Oscar woes:

  Characteristic not known
  Stacktrace:
    [1] error(s::String)
      @ Base ./error.jl:35
    [2] characteristic(R::AffineSchemeOpenSubschemeRing{AffineScheme{QQField, MPolyQuoRing{QQMPolyRingElem}}, AffineSchemeOpenSubscheme{AffineScheme{QQField, MPolyQuoRing{QQMPolyRingElem}}, QQField}})
      @ AbstractAlgebra ~/work/AbstractAlgebra.jl/AbstractAlgebra.jl/src/NCRings.jl:165
    [3] is_trivial(F::AffineSchemeOpenSubschemeRing{AffineScheme{QQField, MPolyQuoRing{QQMPolyRingElem}}, AffineSchemeOpenSubscheme{AffineScheme{QQField, MPolyQuoRing{QQMPolyRingElem}}, QQField}})
      @ AbstractAlgebra ~/work/AbstractAlgebra.jl/AbstractAlgebra.jl/src/Rings.jl:197

[thofma]

The new is_zero(one(R)) test is too good. Some things worked by accident before (due https://github.com/oscar-system/Oscar.jl/issues/4324), but now they won't because there is a genuine zero ring used as the coefficient ring.

[thofma]

Since we don't let Nemo overload polynomial_ring_only anymore, adding the is_trivial check here inside polynomial_ring_only` makes this check too coarse. Each polynomial ring type needs to implement this on their own.

[lgoettgens]

Since we don't let Nemo overload polynomial_ring_only anymore, adding the is_trivial check here inside zpolynomial_ring_only` makes this check too coarse. Each polynomial ring type needs to implement this on their own.

But which of the Nemo coeff types even allows zero rings?

[thofma]

None at the moment, but with this approach here I cannot fix the generic ones without fixing the Nemo ones.

[fingolfin]

So how do we proceed here?

It seems there is some overlap resp. "tension" with PR #1910. Also a bunch of tweaks to this PR here have been suggested but not yet applied.

Would it make sense to split off yet another PR from this with the changes which do not clash with #1910, e.g. for fraction_field ?