Skip to content

Add some ring / field conformance tests#1707

Open
fingolfin wants to merge 1 commit into
thofma:masterfrom
fingolfin:mh/conformance
Open

Add some ring / field conformance tests#1707
fingolfin wants to merge 1 commit into
thofma:masterfrom
fingolfin:mh/conformance

Conversation

@fingolfin
Copy link
Copy Markdown
Contributor

@fingolfin fingolfin commented Dec 19, 2024

... and fix some issues this revealed. In particular "missing" base_ring methods
which the conformance tests kind of assume, but here in some cases "only" base_field methods were available.

Contains #1706 because of overlap.

@thofma
Copy link
Copy Markdown
Owner

thofma commented Dec 19, 2024

I probably misunderstood something, but base_ring is not part of the ring interface. How can a missing base_ring method make the conformance tests fail?

Comment thread src/LocalField/LocalField.jl Outdated
Comment thread test/LocalField/LocalField.jl Outdated
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:
Copy link
Copy Markdown
Collaborator

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

Copy link
Copy Markdown
Contributor Author

@fingolfin fingolfin Jan 13, 2025

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 use 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)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

==

@fingolfin
Copy link
Copy Markdown
Contributor Author

No I don't think you missed anything -- the general conformance tests should not require base_ring. So this is a bug in them. It may still makes sense to test it if available (specifically: if base_ring is there, then so should base_ring_type, and they should agree).

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 hash function; and there are also some more rings defined in Hecke for which it would be IMHO useful to run conformance tests)

Comment thread test/Misc/RelFinField.jl
@testset "RelFinField" begin

@testset "conformance" begin
F = Native.finite_field(3, 3, cached = false)[1]
Copy link
Copy Markdown
Collaborator

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?

Copy link
Copy Markdown
Owner

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.

@thofma
Copy link
Copy Markdown
Owner

thofma commented Dec 20, 2024

Thanks for the clarification. I am also happy to help with missing methods.

@codecov
Copy link
Copy Markdown

codecov Bot commented Jan 24, 2025

Codecov Report

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

Project coverage is 6.46%. Comparing base (c907cf9) to head (364fe92).
Report is 127 commits behind head on master.

Files with missing lines Patch % Lines
src/LocalField/LocalField.jl 0.00% 2 Missing ⚠️
src/Misc/RelFiniteField.jl 0.00% 2 Missing ⚠️
src/NumField/Embedded.jl 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1707      +/-   ##
==========================================
+ Coverage    6.39%    6.46%   +0.06%     
==========================================
  Files         361      361              
  Lines      114249   114640     +391     
==========================================
+ Hits         7310     7414     +104     
- Misses     106939   107226     +287     
Files with missing lines Coverage Δ
src/NumField/Embedded.jl 0.00% <0.00%> (ø)
src/LocalField/LocalField.jl 0.00% <0.00%> (ø)
src/Misc/RelFiniteField.jl 0.00% <0.00%> (ø)

... and 16 files with indirect coverage changes

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

@thofma thofma closed this May 16, 2025
@thofma thofma reopened this May 16, 2025
@fingolfin
Copy link
Copy Markdown
Contributor Author

I forgot about this PR :-( But now it is updated.

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.

3 participants