Add some is_known methods, use one in conformance tests #1996
Conversation
... 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).
| # 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 |
There was a problem hiding this comment.
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?)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Just to say, with either approach to changing the conformance tests, we would have caught the underlying issue for PR #1995
| @@ -21,6 +21,8 @@ canonical_unit(a::NumFieldElem) = a | |||
|
|
|||
| characteristic(F::NumField) = 0 | |||
|
|
|||
| is_known(::typeof(characteristic), F::NumField) = true | |||
There was a problem hiding this comment.
This could be removed again if we go with the fallback approach I suggest. Otherwise we should keep it.
|
I think your comments reveal that it is not clear what
But this also raises the question which functions to add. For example, what is the point of having
On the other hand, a less systematic use of
|
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 outquite 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_knownmethods.One problem that remains is how to prevent people from adding a fast method
for
foobarbut then forgetting to add a matchingis_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_knownbecomes much lessuseful. 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 theymight be controversial, so please have a close look at them specifically.