Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add many @req !is_trivial checks #1857

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Copy link

codecov bot commented Oct 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.17%. Comparing base (0e8c63d) to head (2b22d78).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1857   +/-   ##
=======================================
  Coverage   88.16%   88.17%           
=======================================
  Files         120      120           
  Lines       30219    30227    +8     
=======================================
+ Hits        26643    26652    +9     
+ Misses       3576     3575    -1     

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

@fingolfin
Copy link
Member

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]

src/Fraction.jl Outdated Show resolved Hide resolved
@fingolfin
Copy link
Member

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
Copy link
Collaborator Author

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

@thofma
Copy link
Member

thofma commented Oct 11, 2024

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
Copy link
Collaborator Author

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
Copy link
Member

thofma commented Oct 11, 2024

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

@lgoettgens
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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.

@@ -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."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

So maybe the "currently" can be removed here?

Copy link
Member

@thofma thofma Oct 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -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."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -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."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Forbid zero ring in polynomial ring/series constructions
3 participants