Add some ring / field conformance tests #1707
Conversation
Also remove redundant dense_poly_type methods.
... and fix some issues this revealed
fingolfin
force-pushed
the
mh/conformance
branch
from
aa0f0fa to
2a5446b
Compare
|
I probably misunderstood something, but |
| @@ -134,6 +132,10 @@ end | |||
| # | |||
| ################################################################################ | |||
|
|
|||
| function base_ring(L::LocalField) | |||
There was a problem hiding this comment.
shouldn't this be base_ring(L::LocalField) = base_field(L) instead?
But base_ring was never (intended to be) part of any useful interface, it was just indicating, in recursively defined rings, the previous step. Strictly here, base_ring might even have been the polynomial ring...
base_field, coefficient_ring/field were intended to be more useful
| return sum( [r[i]*b[i] for i in 1:n]) | ||
| end | ||
|
|
||
| # TODO/FIXME: implement isapprox so we can get rid of the following HACK: |
There was a problem hiding this comment.
why? From the docs:
Approximation (floating point and ball arithmetic only)
isapprox(f::MyElem, g::MyElem; atol::Real=sqrt(eps()))so it is not part of the normal ring/field interface I'd say
|
No I don't think you missed anything -- the general conformance tests should not require We'll fix the conformance test suite, and once that's there, I can adjust this PR (there are still a few valid changes in here, like a missing |
| @@ -1,5 +1,12 @@ | |||
| @testset "RelFinField" begin | |||
|
|
|||
| @testset "conformance" begin | |||
| F = Native.finite_field(3, 3, cached = false)[1] | |||
There was a problem hiding this comment.
@thofma I thought we'd rather get rid of the RelFinField?
There was a problem hiding this comment.
I only replaced the ones in the prime decomposition of relative number fields. They might be used somewhere else still.
|
Thanks for the clarification. I am also happy to help with missing methods. |
... and fix some issues this revealed. In particular "missing"
base_ringmethodswhich the conformance tests kind of assume, but here in some cases "only"
base_fieldmethods were available.Contains #1706 because of overlap.