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.

Sign up for GitHub

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

Jump to bottom

Add many @req !is_trivial checks #1857

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
+12 −4
Open

Add many @req !is_trivial checks #1857

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
50eb266
Add many `@req !is_trivial` checks
lgoettgens Oct 10, 2024
2b22d78
Revert matrix changes
lgoettgens Oct 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/Fraction.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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.

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."

return Generic.fraction_field(R; cached=cached)
end

Expand Down
6 changes: 4 additions & 2 deletions src/MPoly.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1508,5 +1508,7 @@ true
Like [`polynomial_ring(R::Ring, s::Vector{Symbol})`](@ref) but return only the
multivariate polynomial ring.
"""
polynomial_ring_only(R::T, s::Vector{Symbol}; internal_ordering::Symbol=:lex, cached::Bool=true) where T<:Ring =
mpoly_ring_type(T)(R, s, internal_ordering, cached)
function polynomial_ring_only(R::T, s::Vector{Symbol}; internal_ordering::Symbol=:lex, cached::Bool=true) where T<:Ring
@req !is_trivial(R) "Zero rings are currently not supported as coefficient ring."
return mpoly_ring_type(T)(R, s, internal_ordering, cached)
end
6 changes: 4 additions & 2 deletions src/NCPoly.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -764,8 +764,10 @@ end
Like [`polynomial_ring(R::NCRing, s::Symbol)`](@ref) but return only the
polynomial ring.
"""
polynomial_ring_only(R::T, s::Symbol; cached::Bool=true) where T<:NCRing =
dense_poly_ring_type(T)(R, s, cached)
function polynomial_ring_only(R::T, s::Symbol; cached::Bool=true) where T<:NCRing
@req !is_trivial(R) "Zero rings are currently not supported as coefficient ring."
return dense_poly_ring_type(T)(R, s, cached)
end

# Simplified constructor

Expand Down
2 changes: 2 additions & 0 deletions src/Residue.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

# Modulus of zero cannot be supported. E.g. A C library could not be expected to
# do matrices over Z/0 using a Z/nZ type. The former is multiprecision, the latter not.
iszero(a) && throw(DomainError(a, "Modulus must be nonzero"))
Expand All @@ -459,6 +460,7 @@ function residue_ring(R::Ring, a::RingElement; cached::Bool = true)
end

function residue_ring(R::PolyRing, a::RingElement; cached::Bool = true)
@req !is_trivial(R) "Zero rings are currently not supported as base ring."
iszero(a) && throw(DomainError(a, "Modulus must be nonzero"))
!is_unit(leading_coefficient(a)) && throw(DomainError(a, "Non-invertible leading coefficient"))
T = elem_type(R)
Expand Down
1 change: 1 addition & 0 deletions src/ResidueField.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

iszero(a) && throw(DivideError())
T = elem_type(R)
S = EuclideanRingResidueField{T}(R(a), cached)
Expand Down
Loading