Add many `@req !is_trivial` checks #1857

lgoettgens · 2024-10-10T15:47:04Z

Resolves #1856.

List of split of changes:

codecov · 2024-10-10T16:03:59Z

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 ⚠️

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

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

🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests

fingolfin · 2024-10-11T07:15:59Z

Thank you!

This triggers a bunch of errors

In Oscar, it seems MPolyQuoRing (and maybe others) doesn't implement characteristic
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]

src/Fraction.jl

fingolfin · 2024-10-11T07:18:43Z

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 · 2024-10-11T07:28:59Z

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

thofma · 2024-10-11T07:48:52Z

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 · 2024-10-11T08:03:17Z

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 · 2024-10-11T08:15:57Z

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

lgoettgens · 2024-10-11T08:31:01Z

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 · 2024-10-11T08:51:50Z

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 · 2024-10-14T13:24:14Z

src/Fraction.jl

@@ -823,6 +823,7 @@ that it will always be returned by a call to the constructor when the same
 base ring $R$ is supplied.
 """
 function fraction_field(R::Ring; cached::Bool=true)
+   @req !is_trivial(R) "Zero rings are currently not supported as coefficient ring."


Can a zero ring have a fraction field at all? What would that be?

So maybe the "currently" can be removed here?

~~I think the zero ring satisfies the universal property of being a fraction field of the zero ring.~~

~~Edit: What I wrote is only correct if you don't insist on the fraction field being a field.~~

Edit edit: I guess the answer is no. What I wrote makes sense for the "total field of fractions", but not for fraction fields.

fingolfin · 2024-10-15T07:24:24Z

src/ResidueField.jl

@@ -490,6 +490,7 @@ residue ring parent object is cached and returned for any subsequent calls
 to the constructor with the same base ring $R$ and element $a$.
 """
 function residue_field(R::Ring, a::RingElement; cached::Bool = true)
+   @req !is_trivial(R) "Zero rings are currently not supported as base ring."


I think here, too, we could remove the "currently", as any quotient of a zero ring can't be a field.

fingolfin · 2024-10-15T07:28:05Z

src/Residue.jl

@@ -450,6 +450,7 @@ to the constructor with the same base ring $R$ and element $a$. A modulus
 of zero is not supported and throws an exception.
 """
 function residue_ring(R::Ring, a::RingElement; cached::Bool = true)
+   @req !is_trivial(R) "Zero rings are currently not supported as base ring."


Since residue_ring can already be a zero ring, I think it is kinda OK to allow a zero ring as base ring? OTOH I also see no strong need to support a zero ring as base ring. But perhaps it allows to make some code more uniform... In any case, we could probably handle it with some code like this:

Suggested change

@req !is_trivial(R) "Zero rings are currently not supported as base ring."

if is_trivial(R)

a = one(R) # base ring is trivial, so we might as well factor out 1

end

Then again: if R is a zero ring, won't the next code line trigger anyway, which throws an error if iszero(A) is true? If that's a case, we may as well keep this line and strengthen it by removing currently

I would keep the error as provided by the PR for now (with the "currently").

I think the line below with the reference to the C library is a bit misleading, as it is not relevant.

thofma · 2024-10-19T10:33:54Z

src/Fraction.jl

@@ -823,6 +823,7 @@ that it will always be returned by a call to the constructor when the same
 base ring $R$ is supplied.
 """
 function fraction_field(R::Ring; cached::Bool=true)
+   @req !is_trivial(R) "Zero rings are currently not supported as coefficient ring."


Suggested change

@req !is_trivial(R) "Zero rings are currently not supported as coefficient ring."

@req !is_trivial(R) "Base ring must not be the zero ring."

fingolfin · 2024-10-24T09:57:52Z

OscarCI and HeckeCI failures: for Oscar, see oscar-system/Oscar.jl#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 · 2024-11-14T20:35:59Z

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 · 2024-11-14T21:24:29Z

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 · 2024-11-18T18:40:15Z

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

thofma · 2024-11-24T11:32:36Z

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 · 2024-11-24T11:46:30Z

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 · 2024-11-24T11:49:45Z

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

lgoettgens requested review from fingolfin and thofma October 10, 2024 15:47

fingolfin reviewed Oct 11, 2024

View reviewed changes

src/Fraction.jl Outdated Show resolved Hide resolved

lgoettgens force-pushed the lg/req-is-not-trivial branch from 254eb62 to e26b4c9 Compare October 11, 2024 08:34

lgoettgens force-pushed the lg/req-is-not-trivial branch from ef779e1 to 2b22d78 Compare October 11, 2024 13:03

fingolfin reviewed Oct 14, 2024

View reviewed changes

fingolfin reviewed Oct 15, 2024

View reviewed changes

thofma reviewed Oct 19, 2024

View reviewed changes

thofma closed this Nov 13, 2024

thofma reopened this Nov 13, 2024

lgoettgens added the breaking label Nov 14, 2024

lgoettgens added 2 commits November 14, 2024 20:18

Add many @req !is_trivial checks

f84179f

Revert matrix changes

37b8a64

thofma force-pushed the lg/req-is-not-trivial branch from 2b22d78 to 37b8a64 Compare November 14, 2024 20:14

thofma mentioned this pull request Nov 15, 2024

feat: more generic is_trivial #1905

Merged

add some characteristic

b274c89

lgoettgens closed this Nov 18, 2024

lgoettgens reopened this Nov 18, 2024

thofma closed this Nov 18, 2024

thofma reopened this Nov 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add many `@req !is_trivial` checks #1857

Add many `@req !is_trivial` checks #1857

lgoettgens commented Oct 10, 2024 •

edited

Loading

codecov bot commented Oct 10, 2024 •

edited

Loading

fingolfin commented Oct 11, 2024

fingolfin commented Oct 11, 2024

lgoettgens commented Oct 11, 2024

thofma commented Oct 11, 2024

lgoettgens commented Oct 11, 2024

thofma commented Oct 11, 2024

lgoettgens commented Oct 11, 2024

lgoettgens commented Oct 11, 2024

fingolfin Oct 14, 2024

thofma Oct 14, 2024 •

edited

Loading

fingolfin Oct 15, 2024

fingolfin Oct 15, 2024

thofma Oct 15, 2024

thofma Oct 19, 2024

fingolfin commented Oct 24, 2024

lgoettgens commented Nov 14, 2024

thofma commented Nov 14, 2024

thofma commented Nov 18, 2024

thofma commented Nov 24, 2024 •

edited

Loading

lgoettgens commented Nov 24, 2024

thofma commented Nov 24, 2024

-   @req !is_trivial(R) "Zero rings are currently not supported as base ring."
+   if is_trivial(R)
+     a = one(R)  # base ring is trivial, so we might as well factor out 1
+   end

	@req !is_trivial(R) "Zero rings are currently not supported as coefficient ring."
	@req !is_trivial(R) "Base ring must not be the zero ring."

Add many @req !is_trivial checks #1857

Are you sure you want to change the base?

Add many @req !is_trivial checks #1857

Conversation

lgoettgens commented Oct 10, 2024 • edited Loading

codecov bot commented Oct 10, 2024 • edited Loading

Codecov Report

fingolfin commented Oct 11, 2024

fingolfin commented Oct 11, 2024

lgoettgens commented Oct 11, 2024

thofma commented Oct 11, 2024

lgoettgens commented Oct 11, 2024

thofma commented Oct 11, 2024

lgoettgens commented Oct 11, 2024

lgoettgens commented Oct 11, 2024

fingolfin Oct 14, 2024

Choose a reason for hiding this comment

thofma Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

fingolfin Oct 15, 2024

Choose a reason for hiding this comment

fingolfin Oct 15, 2024

Choose a reason for hiding this comment

thofma Oct 15, 2024

Choose a reason for hiding this comment

thofma Oct 19, 2024

Choose a reason for hiding this comment

fingolfin commented Oct 24, 2024

lgoettgens commented Nov 14, 2024

thofma commented Nov 14, 2024

thofma commented Nov 18, 2024

thofma commented Nov 24, 2024 • edited Loading

lgoettgens commented Nov 24, 2024

thofma commented Nov 24, 2024

Add many `@req !is_trivial` checks #1857

Add many `@req !is_trivial` checks #1857

lgoettgens commented Oct 10, 2024 •

edited

Loading

codecov bot commented Oct 10, 2024 •

edited

Loading

thofma Oct 14, 2024 •

edited

Loading

thofma commented Nov 24, 2024 •

edited

Loading