Skip to content

Add many @req !is_trivial checks #1857

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

Open
lgoettgens wants to merge 3 commits into
base: master
Choose a base branch
from
+10 −4
Open

Add many @req !is_trivial checks #1857

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b570971
Add many `@req !is_trivial` checks
lgoettgens Oct 10, 2024
8d3cd9b
Update src/generic/Misc/Localization.jl
fingolfin Dec 15, 2025
998c31f
Update src/generic/Misc/Localization.jl
fingolfin Dec 15, 2025
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
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 @@ -1655,5 +1655,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
Copy link
Member

@fingolfin fingolfin Dec 15, 2025

Choose a reason for hiding this comment

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

This of course conflicts with PR #1729 (no biggie, just want to make it explicit -- we'll have to plan in time to adjust depending on the order we merge these in)

@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 @@ -756,8 +756,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 =
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 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 @@ -445,6 +445,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

@fingolfin fingolfin Oct 15, 2024

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

@thofma thofma Oct 15, 2024

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.
Comment on lines 449 to 450
Copy link
Member

@fingolfin fingolfin Nov 28, 2024

Choose a reason for hiding this comment

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

Suggested change
# 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 @@ -454,6 +455,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
Loading