AbstractAlgebra.jl icon indicating copy to clipboard operation
AbstractAlgebra.jl copied to clipboard

Proof-of-concept: _implements trait

Open fingolfin opened this issue 11 months ago • 3 comments

Proof of concept of a "trait" that allows checking for some unary functions whether they are implemented for a given type. See also https://github.com/oscar-system/Oscar.jl/issues/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.

fingolfin avatar Jan 15 '25 00:01 fingolfin

Codecov Report

Attention: Patch coverage is 0% with 28 lines in your changes missing coverage. Please review.

Project coverage is 88.30%. Comparing base (e877196) to head (67b766a).

Files with missing lines Patch % Lines
src/ConformanceTests.jl 0.00% 8 Missing :warning:
src/algorithms/GenericFunctions.jl 0.00% 5 Missing :warning:
src/MPoly.jl 0.00% 3 Missing :warning:
src/Poly.jl 0.00% 3 Missing :warning:
src/generic/LaurentMPoly.jl 0.00% 3 Missing :warning:
src/MatRing.jl 0.00% 2 Missing :warning:
src/algorithms/LaurentPoly.jl 0.00% 2 Missing :warning:
src/FreeAssociativeAlgebra.jl 0.00% 1 Missing :warning:
src/NCRings.jl 0.00% 1 Missing :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1957      +/-   ##
==========================================
- Coverage   88.37%   88.30%   -0.08%     
==========================================
  Files         126      126              
  Lines       31639    31667      +28     
==========================================
  Hits        27962    27962              
- Misses       3677     3705      +28     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

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

codecov[bot] avatar Jan 15 '25 01:01 codecov[bot]

I would be in favor of moving this to ConformanceTests for now.

thofma avatar Jan 15 '25 08:01 thofma

Should I move this to ConformanceTests._implements or ConformanceTests.implements ?

fingolfin avatar Jan 29 '25 23:01 fingolfin