-
Notifications
You must be signed in to change notification settings - Fork 67
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add some ring / field conformance tests #1707
base: master
Are you sure you want to change the base?
Conversation
aa0f0fa
to
2a5446b
Compare
I probably misunderstood something, but |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I never really questioned the user of isapprox
in the conformance tests, I assumed that Dan had checked this was the right thing to use when he added them.
I'd be happy to use something else then, but what? We currently have this helper in the conformance tests:
# helper
function equality(a, b)
if is_exact_type(typeof(a)) && is_exact_type(typeof(b))
return a == b
else
return isapprox(a, b)
end
We use it to implement tests like these:
@test equality(a^4, a*a*a*a)
@test equality((a + b) + c, a + (b + c))
@test equality(a + b, b + a)
@test equality(a - c, a + (-c))
What would be a way to compare two QAdicFieldElem
in a way that would satisfy such tests that deals with the problem that e.g. a^4
and a*a*a*a
may produce slightly different results (e.g. perhaps a^4
is "more" accurate)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
==
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thofma I thought we'd rather get rid of the RelFinField?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
2a5446b
to
364fe92
Compare
... and fix some issues this revealed. In particular "missing"
base_ring
methodswhich the conformance tests kind of assume, but here in some cases "only"
base_field
methods were available.Contains #1706 because of overlap.