Skip to content

Add ConformanceTests._implements trait for internal use#1957

Open
fingolfin wants to merge 3 commits intoNemocas:masterfrom
fingolfin:mh/implements
Open

Add ConformanceTests._implements trait for internal use#1957
fingolfin wants to merge 3 commits intoNemocas:masterfrom
fingolfin:mh/implements

Conversation

@fingolfin
Copy link
Member

@fingolfin fingolfin commented Jan 15, 2025

Proof of concept of a "trait" that allows checking for some unary functions whether they are implemented for a given type. See also oscar-system/Oscar.jl#4394 for background.

So e.g. one can write _implements(ZZRingElem, factor) to determine whether factor is implemented.

This is different from applicable and hasmethod in that it allows dealing with "generic" methods that delegate to other functionality. For example, we have an implementation of is_nilpotent for arbitrary polynomials, but it delegates to is_nilpotent for the coefficient ring. So we can only consider it as "implemented" if it is implemented for the coefficient ring.

Right now the way this is implemented it requires manual work to "model" these relations explicitly, and so in general it won't be reliable for "arbitrary" unary functions. Thus I named it _implements to mark it as "internal sharp tool, use with caution".

My personal main use for this is in the conformance test suite. But I think it might also be useful for some algorithms which may be able to do something "better" if e.g. factor is available. Though of course such algorithms could often work by wrapping the factor call(s) into try/catch blocks. But this approach is not useful for the conformance tests: trying to put them inside a @test leads to ugliness like this. It also isn't sufficient to e.g. call factor(zero(R)) inside try/catch to determine if factor is available for elements of the ring R because there are some "generic methods" that work only on some arguments (e.g. zero...) and so this test may give misleading results.

If we follow PR #1954 and turn the conformance tests into a package extension, this method could also be moved into the ConformanceTest module if nobody is interested in this functionality beyond the conformance tests. DONE


Update: now contains and hence closes #1947

@codecov
Copy link

codecov bot commented Jan 15, 2025

Codecov Report

❌ Patch coverage is 29.82456% with 40 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.04%. Comparing base (7fa5e04) to head (68295f1).
⚠️ Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
ext/TestExt/Rings-conformance-tests.jl 40.74% 16 Missing ⚠️
src/algorithms/GenericFunctions.jl 0.00% 5 Missing ⚠️
src/ConformanceTests.jl 50.00% 4 Missing ⚠️
src/MPoly.jl 0.00% 3 Missing ⚠️
src/Poly.jl 0.00% 3 Missing ⚠️
src/generic/LaurentMPoly.jl 0.00% 3 Missing ⚠️
src/MatRing.jl 0.00% 2 Missing ⚠️
src/algorithms/LaurentPoly.jl 0.00% 2 Missing ⚠️
src/FreeAssociativeAlgebra.jl 0.00% 1 Missing ⚠️
src/NCRings.jl 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1957      +/-   ##
==========================================
- Coverage   88.14%   88.04%   -0.11%     
==========================================
  Files         126      126              
  Lines       32093    32149      +56     
==========================================
+ Hits        28289    28306      +17     
- Misses       3804     3843      +39     

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

Comment on lines 479 to 481
# TODO: deal with type parameters: if `T` is `FreeAssociativeAlgebraElem{ZZRingElem}`
# and `f` has a method for `FreeAssociativeAlgebraElem` then we should still consider
# this a match.
Copy link
Member

Choose a reason for hiding this comment

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

What about the case that there is some method for PolyRing and we have a QQPolyRing. Is this a match?
What about T = QQPolyRing and f(::PolyRing{QQFieldElem})?
I am not sure if there is a deterministic and consistent semantic of match that you want here

Copy link
Member Author

Choose a reason for hiding this comment

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

No, a method for PolyRing should not "match" QQPolyRing within _implements_directly. The idea is that this function is meant for methods that match a "concrete" type: you are not going to define e.g. a factor(::QQPolyRing) just to also throw a NotImplementedError.

I do think this is well-defined: The type T is a concrete type here. What I can do (well, at least in theory) is to "remove parameters" from it, to e.g. turn Vector{Int} into Vector, and then compare to the methods in meth (possibly after removing parameters there, too).

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess using typename(T).wrapper might be one option...

@thofma

This comment was marked as resolved.

@fingolfin

This comment was marked as resolved.

@fingolfin fingolfin added the release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes label Jan 31, 2025
... to allow fine control over which things to test in the conformance
tests and which not.
@fingolfin fingolfin changed the title Proof-of-concept: _implements trait Add ConformanceTests._implements trait for internal use Feb 11, 2026
Also improve is_nilpotent for matrices / matrix ring elements
to work if the base ring is trivial.
@fingolfin fingolfin marked this pull request as ready for review February 11, 2026 23:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants