Skip to content

Add some is_known methods, use one in conformance tests #1996

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

Draft
fingolfin wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Add some is_known methods, use one in conformance tests #1996

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
2943d82
Use is_known in conformance tests
fingolfin Feb 13, 2025
10ced96
Add a fallback for is_known(characteristic, ::NCRing)
fingolfin Feb 13, 2025
7fff881
Apply suggestion from @fingolfin
fingolfin Feb 13, 2026
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
11 changes: 11 additions & 0 deletions ext/TestExt/Rings-conformance-tests.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,10 +44,14 @@ function test_NCRing_interface(R::AbstractAlgebra.NCRing; reps = 15)

# some rings don't support characteristic and raise an exception (see issue #993)
try ch = characteristic(R)
@test AbstractAlgebra.is_known(characteristic, R)
@test iszero(R(characteristic(R)))
@test iszero(characteristic(R) * one(R))
@test iszero(one(R) * characteristic(R))
catch
# could not compute characteristic, so verify that is_known
# reflects this
@test AbstractAlgebra.is_known(characteristic, R) == false
end
end

Expand Down Expand Up @@ -301,6 +305,13 @@ function test_Field_interface(R::AbstractAlgebra.Field; reps = 15)

test_Ring_interface(R, reps = reps)

# We implicitly assume all genuine fields (i.e. of type `Field`, not
# just rings that happen to be fields) know their characteristic. So
# test for that. We may relax this in the future if we have need for it.
# But for now if the next tests fail this usually means someone forgot
# to implement `characteristic` for their ring type properly.
@test AbstractAlgebra.is_known(characteristic, R) == true

@test iszero(R(characteristic(R)))
@test iszero(characteristic(R) * one(R))
@test iszero(one(R) * characteristic(R))
Expand Down
7 changes: 7 additions & 0 deletions src/NCRings.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,13 @@ function characteristic(R::NCRing)
error("Characteristic not known")
end

# All rings are supposed to implement characteristic if they can. If they
# can not, or only can do it with an expensive computation (e.g. a Groebner
# basis), then instead they should implement an `is_known` method indicating
# this. To enforce this via the ring conformance tests, we add this default
# method for `is_known(characteristic, ::NCRing)`
is_known(::typeof(characteristic), ::NCRing) = true
Copy link
Copy Markdown
Member Author

@fingolfin fingolfin Feb 13, 2025

Choose a reason for hiding this comment

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

This is also demonstrated by PR #1995 which @thofma created while I worked on this PR: it adds new characteristic and is_finite methods. With the above is_known, we don't need to add another is_known method for RationalFunctionField.

If we agree on this approach I could add similar fallbacks for is_finite and is_trivial (and is_perfect?)

Copy link
Copy Markdown
Member Author

@fingolfin fingolfin Feb 13, 2025

Choose a reason for hiding this comment

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

OK I just realized that in the final is_known implementation the "default" is actually to throw an exception for is_known. We can of course leave it at that, and just add a test to the conformance test suite that invokes is_known(characteristic, R) on every ring R it is invoked for.

The downside then is that everyone implementing a ring has to add the "missing" is_known methods, even if they always return true. But perhaps that's the best approach anyway. I am really am open to either approach.

Copy link
Copy Markdown
Member Author

@fingolfin fingolfin Feb 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Just to say, with either approach to changing the conformance tests, we would have caught the underlying issue for PR #1995


###############################################################################
#
# One and zero
Expand Down
Loading