More unit test improvements

[mikeingold]

Complete

• Added new style of tests for Ring, Rope, Segment
• Added tests to improve code coverage from 84% to 91%
• Added tests where argument FP is Float32 or BigFloat, discovered issues leading to:
  • Some loss of precision with Float32 (loosened test atol parameter)
  • Type instability, i.e. Float32 results are promoted to Float64
    • Documented in Issue Floating point argument FP not working for Float32 #74
  • Added notes to integral docstring

Future Work (Other PR's)

• Add a method for manual tracking of which combinations of integral argument types have tests implemented
• Re-consider whether argument FP should be a kwarg, i.e. integral(f, geometry, settings; FP=BigFloat)

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.93%. Comparing base (8813827) to head (9a4a23e).
Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #73      +/-   ##
==========================================
+ Coverage   83.85%   90.93%   +7.08%     
==========================================
  Files           7        7              
  Lines         353      353              
==========================================
+ Hits          296      321      +25     
+ Misses         57       32      -25     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[mikeingold]

@JoshuaLampert do you have time for a review?

[JoshuaLampert]

Not right now, but tomorrow I should find a couple of minutes.

[JoshuaLampert]

Looks mostly good to me. I left some minor comments and questions below.
I see a lot of commits in the PR, which indicates to me that you don't run the tests locally (?). While that's of course totally fine, it might still be more convenient and probably faster to run the tests during development locally, such that CI doesn't need to be triggered each time again to get some response. To make this more convenient we could think about integrating tests with TestItems.jl and TestItemRunner.jl, which can be used to run only certain test items locally individually without needing to run the whole test workflow (this assumes you develop within VSCode). This makes the development of tests much easier and faster. Meshes.jl integrated this recently and I also played around with it recently. I really like the workflow and can recommend it. If you agree that this is also worth implementing in MeshIntegrals.jl, I can also set up a PR for that. Now that I finished writing this, I just saw you also mention TestItems.jl in #67 😅

[mikeingold]

I see a lot of commits in the PR, which indicates to me that you don't run the tests locally (?). While that's of course totally fine, it might still be more convenient and probably faster to run the tests during development locally, such that CI doesn't need to be triggered each time again to get some response.

I try not to dev this way, but I've been away from my workstation a lot lately and had to push these commits from GitHub's browser file editor 😅.

If you agree that this is also worth implementing in MeshIntegrals.jl, I can also set up a PR for that. Now that I finished writing this, I just saw you also mention TestItems.jl in #67 😅

I'm definitely interested in implementing TestItems.jl soon but was planning to get all of the tests updated first. At some point earlier this year I came up with a scheme for trying to generate @tests on all valid combinations {integral function name, integrand output type, Meshes.Geometry sub-type, IntegrationAlgorithm}. It sounded like a good idea in principle, and provided a big boost in test coverage at a time, but: it relies on Meshes.measure as a benchmark, only covers cases with unit integrand (p -> one(T)), tends to bloat the test run-times with some potentially-redundant coverage, and tended to slightly obscure what actually went wrong when a test might fail. My plan has been to transition toward a style of tests more like what Meshes uses, i.e. modular blocks of concrete tests with analytic solutions.

One of the things I am conflicted about conceptually, though, is how to manage true test coverage this way. For example, pretty much any individual call of the form integral(f, geometry, ::HAdaptiveCubature) now relies on the same method, so one applicable @test is enough to satisfy CodeCov for it. The note in the Future Work block above references a thought I had to manually populate some kind of table-like data structure, with entries in each appropriate @testset to indicate which combinations are exercised. It isn't the most elegant solution, though, so I'm all ears if you have other ideas.

[mikeingold]

I've noticed periodic issues where the macOS CI alone will error and it's usually something odd not related to the actual package, so I was about to re-run the failing test, but I noticed that this one has a really odd error being thrown seemingly out of Meshes.

This function (inside another function in utils.jl) generates a parametric call (::Ball{3})(rho, theta, phi)

https://github.com/mikeingold/MeshIntegrals.jl/blob/8349dbc64a42712ef151d929337c65ec5e07a848/src/utils.jl#L29-L35

which at this line in Meshes.jl for some reason generates an error trying to convert to an Int

https://github.com/JuliaGeometry/Meshes.jl/blob/c8b2b936808a2b0810bf1c0130a077deb2a10d3c/src/geometries/primitives/ball.jl#L62C3-L62C30

Edit: just pinged Julio on Zulip

[JoshuaLampert]

I've seen this error already before in a completely different context here (also only on Mac):
JoshuaLampert/DispersiveShallowWater.jl/actions/runs/10811172112/job/29990016675?pr=150#step:7:8511
This is indeed very weird, but not related to anything we should worry too much about, I think.

[JoshuaLampert]

I'm definitely interested in implementing TestItems.jl soon but was planning to get all of the tests updated first.

I see, but I think having TestItems.jl running might make the development of the new tests more convenient. Thus, I would think implementing TestItems.jl first and updating the remaining tests after that might be worth considering (assuming, of course, that you use your workstation in future PRs again because otherwise TestItems.jl doesn't make anything more convenient 😅).

[mikeingold]

I've seen this error already before in a completely different context here (also only on Mac): JoshuaLampert/DispersiveShallowWater.jl/actions/runs/10811172112/job/29990016675?pr=150#step:7:8511 This is indeed very weird, but not related to anything we should worry too much about, I think.

Interesting. I wonder if this is some obscure bug in Base. Looks like your example triggers here.

https://github.com/JuliaLang/julia/blob/911e02558d0c145a192facd28808b68e157aa5af/base/special/trig.jl#L841

[mikeingold]

I'm definitely interested in implementing TestItems.jl soon but was planning to get all of the tests updated first.

I see, but I think having TestItems.jl running might make the development of the new tests more convenient. Thus, I would think implementing TestItems.jl first and updating the remaining tests after that might be worth considering (assuming, of course, that you use your workstation in future PRs again because otherwise TestItems.jl doesn't make anything more convenient 😅).

That was just the order-of-events I was planning to work personally. If you're up for implementing TestItems once this PR is merged, I'm all for it.

[mikeingold]

I think all reviews have now been addressed. Let me know if I'm missing anything.

[JoshuaLampert]

Interesting. I wonder if this is some obscure bug in Base. Looks like your example triggers here.

JuliaLang/julia@911e025/base/special/trig.jl#L841

Yes, might be.

[JoshuaLampert]

LGTM!