Skip to content
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

Test field interface conformance for arb types #1886

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

fingolfin
Copy link
Member

No description provided.

Comment on lines +15 to +17
function equality(a::T, b::T) where T <: Union{ArbFieldElem,RealFieldElem,AcbFieldElem,ComplexFieldElem}
return isequal(a, b)
end
Copy link
Member Author

@fingolfin fingolfin Oct 8, 2024

Choose a reason for hiding this comment

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

this is a gross hack that I used to pass some tests (so that equality(a, deepcopy(a)) holds -- or for that matter, equality(a, a)).

But what we really need is a working isapprox method, see issue #1877

Copy link
Collaborator

Choose a reason for hiding this comment

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

could you split this up into the four files? In the way this is currently, one cannot just run "Complex-test.jl" on its own as it needs this definition.

And I noticed that there is == and isequal for these types, and they call different flint functions. What is the difference and why did you choose isequal for this case?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry my comment was confusing written (I've edited it now). The point is that equality by default is implemented using ==. But for arb types, in general a == a is not satisfied.

Hence this hack of using isequal which at least ensures equality(a, deepcopy(a)) and equality(a, a) both return true. But it doesn't really solve the problem, we need isapprox.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants