Conversation
Benchmark Results (Julia vlts)Time benchmarks
Memory benchmarks
|
Benchmark Results (Julia v1)Time benchmarks
Memory benchmarks
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1327 +/- ##
==========================================
+ Coverage 85.50% 85.62% +0.12%
==========================================
Files 199 200 +1
Lines 6376 6409 +33
==========================================
+ Hits 5452 5488 +36
+ Misses 924 921 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Regarding support for multiple quadrature libraries, we need to think if it is really necessary. If it is necessary, we could start by creating an IntegrationInterface.jl package after this PR is merged, similar to the DifferentiationInterface.jl approach. |
There is Integrals.jl, which basically provides exactly that. |
I think it would be nice because Gauss-Kronrod should be more efficient to get a certain accuracy in 1D and H-adaptive cubature should be better suited in multi D. |
I saw it, but it is not at the same level of DifferentiationInterface.jl. It includes a bunch of dependencies, including backends and SciML stuff. |
|
Please feel free to add tests if you have some already written in MeshIntegrals.jl. I will try to continue the work over the weekend, and next week we should have a final version to review 👍🏽 |
JoshuaLampert
left a comment
There was a problem hiding this comment.
I have two questions:
- How do we deal with the specializations? Do we add them in (a) later PR(s)?
- How should we write tests? In MeshIntegrals.jl we have quite extensive tests, which take some time though.
- Should we have tests for every (supported) geometry? I think we should because just because the integral works fine for one or a few geometries does not mean it works for all, see the need for the specializations. I think we can use the non-trivial (i.e., we do not just integrate a constant function) analytical tests we have in the test suite of MeshIntegrals.jl.
- Should we use some test infrastructure to automate the testing and reduce code duplication and redundancy like in MeshIntegrals, see here or should we rather go with a straightforward approach and testing each geometry individually?
We should add them here as well. I will try to find the time tomorrow to finish up the additions 👍🏽
Yes. We should have full coverage.
I think we can be more straightforward testing each geometry individually. It gives us more freedom to add more tests later without affecting all other tests. |
|
@JoshuaLampert I've ported some tests from MeshIntegrals.jl to see if everything was working as expected, but the results are not matching. Do you have any idea of what might be happening? Could it be related to the default backend? Should we adjust the tests? |
They are very likely failing due to the low number of Gauss-Legendre points you are using by default, i.e., related to your comment #1327 (comment). I think for general non-linear functions, we should provide a much higher value for |
|
Also I just see from the failing |
Got it! I will try to find a small enough number that gives us a good default for moderately nonlinear functions.
We want to preserve the type of the input. I thought the implementation would guarantee that, but maybe the backend is promoting the arguments? I will double check. |
I think the problem is that the Gauss-Legendre nodes and weights provided by FastGaussQuadrature.jl are |
|
I've increased the default order to After we sort out the source of the issue, we could pick something like 3 by default (exact approximation of polynomials of degree 5). It is hard to imagine applications where this default wouldn't be enough. |
|
Could the change in results be related to the change of variable formula? I used |
Hm, currently, I don't see another semantic difference between the two implementations. So give it a try? |
|
What is different is the finite difference scheme used to compute the |
That was it. I will adjust the tests to be more explicit about the |
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
|
@JoshuaLampert I've copied/pasted the tests with infinite geometries from MeshIntegrals.jl, but they are failing too. Perhaps I messed up the change of variables. Can you spot any issue with the current implementation? Did I use the correct change of variable? I've marked the tests with |
|
On a brief look, I did not spot anything wrong with the infinite geometries. The transformations look correct to me. I would need to dig deeper and debug together with MeshIntegrals.jl to see where they first differ. I don't have the right now though. |
|
@JoshuaLampert we have all methods in place. The tests for |
JoshuaLampert
left a comment
There was a problem hiding this comment.
Thanks! This looks mostly good to me. I left a few minor suggestions/questions below.
- A test for
SimpleMeshis still missing. - Should we also add a test that the floating point type is preserved by
integralor do you think this is already implicitly tested for by the accuracy tests? - I don't see right now why the geometries, which need a custom transformation fail. It seems like they have the same reason why they fail. So to debug this I would pick the simplest one (I guess
Ray), add some debug statements at different steps in Meshes.jl and MeshIntegrals.jl and see where they differ to see what might be the problem. Did you already try something like this? I do not have the time to do that right now though (I am on vacation). Otherwise maybe AI also has an idea?
| # specialize quadrangle for performance | ||
| localintegral(fun, quad::Quadrangle; n=3) = _uvwintegral(fun, quad, n) |
There was a problem hiding this comment.
Why does this specialization improve performance?
There was a problem hiding this comment.
We had a method for general Polygon that got erased. The method for general polygon relied on discretization, and this method for Quadrangle relies on direct sampling with the parametric function. I will review the code to see if this method is still necessary.
Will add it right after we fix Triangle and Tetrahedron
I think we can assume the implementation preserves the number type.
Will check that. |
Ports
integralfrom MeshIntegrals.jl and addslocalintegralfor integration of functions that are defined locally in terms of parametric coordinates.@JoshuaLampert do you have suggestions before I start writing tests?