Skip to content

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

Draft
fingolfin wants to merge 3 commits intomasterfrom
mh/is_known-characteristic
Draft

Add some is_known methods, use one in conformance tests#1996
fingolfin wants to merge 3 commits intomasterfrom
mh/is_known-characteristic

Conversation

@fingolfin
Copy link
Copy Markdown
Member

@fingolfin fingolfin commented Feb 13, 2025

  • Implement is_known(characteristic, ::NCRing) (already merged)
  • Implement is_known(is_perfect, ::Field) (already merged)
  • Implement is_known(is_finite, ::NCRing)
  • Use is_known in conformance tests
  • Add a fallback for is_known(characteristic, ::NCRing)

This PR is my own experiment to see how much work it takes to make is_known
(and related functions like my _implements / can_compute) work. Turns out
quite a few methods need to be added here in AA, but luckily these then cover
many different ring implementations. Only further "leaf ring types" need to
provide further is_known methods.

One problem that remains is how to prevent people from adding a fast method
for foobar but then forgetting to add a matching is_known(foobar, x)
method. This can happen easily, and in fact it will be the case right now for
many rings in Nemo/Hecke/Oscar simply because they existed before we created
is_known.

We really want to avoid that because otherwise is_known becomes much less
useful. One way to tackle this would be by using the conformance tests to
enforce this to a level.

The final two commits in this PR try to do this for characteristic. But they
might be controversial, so please have a close look at them specifically.

Comment thread src/NCRings.jl
# 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

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

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

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

Comment thread src/NumFields.jl

characteristic(F::NumField) = 0

is_known(::typeof(characteristic), F::NumField) = true
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This could be removed again if we go with the fallback approach I suggest. Otherwise we should keep it.

@thofma
Copy link
Copy Markdown
Member

thofma commented Feb 13, 2025

I think your comments reveal that it is not clear what is_known will be used for. In the context here, it seems that is_known is becoming part of the interface for rings/fields, but without writing down what is explicitly required:

  • every field type must implement is_known(typeof(characteristic), F::Field)? Or
  • for every field type characteristic(F::Field) must be "fast"?

But this also raises the question which functions to add. For example, what is the point of having is_known(::typeof(characteristic), F::Field)?

  • Why would we want characterstic to be available via is_known, or
  • do we want everything available via is_known?

On the other hand, a less systematic use of is_known (and this is how it is/was used before in the various customs variations of is_known) is:

  • there is no formal interface and the developers know what they doing?

@fingolfin fingolfin force-pushed the mh/is_known-characteristic branch 2 times, most recently from e876dd0 to 56144fe Compare January 8, 2026 16:52
Comment thread src/LaurentMPoly.jl Outdated
characteristic(R::LaurentMPolyRing) = characteristic(base_ring(R))

is_finite(R::LaurentMPolyRing) = is_trivial(base_ring(R)) || (nvars(R) == 0 && is_finite(base_ring(R)))
is_known(::typeof(is_finite), R::LaurentMPolyRing) = is_known(is_trivial, base_ring(R)) || (nvars(R) == 0 && is_known(is_finite, base_ring(R)))
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is wrong, see #2270 (comment)

@fingolfin fingolfin changed the title Add some is_known methods, use one in conformance tests Add some is_known methods, use one in conformance tests Feb 11, 2026
@fingolfin fingolfin marked this pull request as draft February 11, 2026 22:15
@fingolfin fingolfin force-pushed the mh/is_known-characteristic branch from 56144fe to 51f2ffb Compare February 13, 2026 17:07
Comment thread ext/TestExt/Rings-conformance-tests.jl
... to force ring implementations to provide at least one of
characteristic(R) and is_known(characteristic, R) if they want to
pass the conformance tests.

This is the only way I can see to stop rings from implementing
characteristic for their ring type but forgetting to also provide
a matching is_known(characteristic, R).
@fingolfin fingolfin force-pushed the mh/is_known-characteristic branch from 7f73519 to 7fff881 Compare February 19, 2026 22:11
@codecov
Copy link
Copy Markdown

codecov bot commented Feb 19, 2026

Codecov Report

❌ Patch coverage is 75.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 88.24%. Comparing base (daf3f0c) to head (7fff881).
⚠️ Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
ext/TestExt/Rings-conformance-tests.jl 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1996      +/-   ##
==========================================
+ Coverage   88.20%   88.24%   +0.04%     
==========================================
  Files         127      127              
  Lines       32847    32850       +3     
==========================================
+ Hits        28972    28989      +17     
+ Misses       3875     3861      -14     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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.

2 participants