Adds integral evaluation on Bezier curves/bounded regions by jcs15c · Pull Request #848 · llnl/axom

jcs15c · 2022-06-21T19:27:56Z

This PR is a feature
It adds the evaluate_integrals.hpp file to primal/operators, which contains methods to
- Evaluate scalar line integrals on collections of Bezier curves.
- Evaluate vector field line integrals on collections of Bezier curves.
- Evaluate 2D area integrals for regions defined by Bezier curves.
It adds additional tests in primal for the above methods.
It adds an optional dependency for primal on mfem, used to evaluate quadrature algorithms.

edit: See https://en.wikipedia.org/wiki/Line_integral#Line_integral_of_a_scalar_field and https://en.wikipedia.org/wiki/Line_integral#Line_integral_of_a_vector_field for information about the different types of line integrals.

kennyweiss

Thanks @jcs15c -- this is great!

I added some suggestions for minor changes.
Please also add a line to the RELEASE-NOTES.

src/axom/primal/CMakeLists.txt

src/axom/primal/operators/detail/evaluate_integral_impl.hpp

src/axom/primal/operators/evaluate_integral.hpp

src/axom/primal/operators/detail/evaluate_integral_impl.hpp

src/axom/primal/tests/CMakeLists.txt

…ncy of primal on mfem

Co-authored-by: Kenny Weiss <kennyweiss@users.noreply.github.com>

kennyweiss

Thanks @jcs15c -- this is really nice!

Tag: @davidgunderman

kennyweiss · 2022-06-23T01:02:24Z

src/axom/primal/operators/detail/evaluate_integral_impl.hpp

+ */
+inline double evaluate_line_integral_component(
+  const primal::BezierCurve<double, 2>& c,
+  std::function<double(Point2D)> integrand,


I think the lambda should be passed by r-value ref (here and elsewhere):

Suggested change

std::function<double(Point2D)> integrand,

std::function<double(Point2D)>&& integrand,

(@kanye-quest -- could you please confirm?)

I'm very unclear on the details, but passing the std::function's by R-value reference is causing some issues with some to-be-implemented winding number code. Specifically, if we pass to evaluate_line_integral a lambda function that is the return value for some other function (see below), then evaluate_line_integral_component can no longer fit the argument to either overloaded definition. This doesn't happen when the std::function's are passed by constant reference, or when the lambda function is not the return value for another function, so I am unsure how to keep this as general as possible.

// An example of function that returns argument to evaluate_line_integral std::function<Vector2D(Point2D)> get_vector_field(Point2D p) { return [p](Point2D x) -> Vector2D { return Vector2D({p[1], p[0]}); }; }

Thanks -- ok, sounds like something to think about and try to fix in the future.

This might have something to do with this function (and the one below) accepting an std::function instead of a Lambda&& like in the other methods. The latter is a universal reference, so it can bind to both lvalues and rvalues.

For what it's worth, I think accepting Lambda&& might be better from an inlineability perspective -- it would be much more difficult for the compiler to inline through a type-erased std::function.

publixsubfan

LGTM, and thanks for the tests! Just a few comments to think about.

publixsubfan · 2022-06-24T00:30:49Z

src/axom/primal/CMakeLists.txt

+   ELEMENTS operators/evaluate_integral.hpp
+            operators/detail/evaluate_integral_impl.hpp
+   IF       MFEM_FOUND 
+    )


Nit: alignment?

publixsubfan · 2022-06-24T00:47:58Z

src/axom/primal/operators/detail/evaluate_integral_impl.hpp

+ */
+inline double evaluate_line_integral_component(
+  const primal::BezierCurve<double, 2>& c,
+  std::function<double(Point2D)> integrand,


This might have something to do with this function (and the one below) accepting an std::function instead of a Lambda&& like in the other methods. The latter is a universal reference, so it can bind to both lvalues and rvalues.

For what it's worth, I think accepting Lambda&& might be better from an inlineability perspective -- it would be much more difficult for the compiler to inline through a type-erased std::function.

publixsubfan · 2022-06-24T00:49:07Z

src/axom/primal/operators/evaluate_integral.hpp

+  // Get the quadrature for the line integral.
+  //  Quadrature order is equal to 2*N - 1
+  quad_Q = &(my_IntRules.Get(mfem::Geometry::SEGMENT, 2 * npts_Q - 1));
+  quad_P = &(my_IntRules.Get(mfem::Geometry::SEGMENT, 2 * npts_P - 1));


Minor: any reason to use pointers to the IntegrationRules instead of references?

Only that the quest test I used for reference used them, changing it to references makes it much cleaner. Thank you!

agcapps · 2022-06-24T15:48:48Z

src/axom/primal/operators/evaluate_integral.hpp

+ * Regions Bounded by Rational Parametric Curves" by David Gunderman et al.
+ */


I appreciate this whole comment! I see you wrote "with ... rules supplied by MFEM," as is accurate. Please add the additional note, to make things extra clear:

Suggested change

* Regions Bounded by Rational Parametric Curves" by David Gunderman et al.

*/

* Regions Bounded by Rational Parametric Curves" by David Gunderman et al.

*

* \note This requires the MFEM third-party library.

*/

This Doxygen mirrors the compile-time checks around line 28 of this file and line 15 of evaluate_integral_impl.hpp

…com/LLNL/axom into feature/spainhour/bezier_quadrature

agcapps · 2022-06-24T16:56:12Z

src/axom/primal/tests/primal_integral.cpp

+  EXPECT_NEAR(evaluate_line_integral(connected_curve, transc_integrand, npts),
+              -0.574992518405,
+              abs_tol);
+}


Please add a test with disconnected line segments, as this is noted as possible in the doxygen for evaluate_line_integral. Perhaps displace the middle, linear segment by (0, 2) or some such.

agcapps · 2022-06-24T16:56:54Z

src/axom/primal/tests/primal_integral.cpp

+  EXPECT_NEAR(evaluate_line_integral(parabola_shape, winding_field, npts),
+              1.0,
+              abs_tol);
+}


Please add a test for disconnected segments.

agcapps

Thanks for this!

jcs15c added enhancement New feature or request Primal Issues related to Axom's 'primal component labels Jun 21, 2022

jcs15c requested a review from kennyweiss June 21, 2022 19:27

jcs15c self-assigned this Jun 21, 2022

jcs15c removed the enhancement New feature or request label Jun 21, 2022

kennyweiss requested review from agcapps, publixsubfan and white238 June 22, 2022 20:08

kennyweiss requested changes Jun 22, 2022

View reviewed changes

jcs15c and others added 12 commits June 22, 2022 16:30

Added evaluate_integral.hpp, changed CMake file, add optional dependa…

4ecd1b9

…ncy of primal on mfem

Added test for evaluate_integral methods, changed CMake file

3aa6a04

Added copyright info

52a24a7

Corrected style

ff23aad

Fixed style, slic logging

2095c3e

Fixed lambda function warning

b5496b1

Fixed dependency of primal::evaluate_integral on mfem in build system

67212d7

Add implementation file to minimize interface, change CMake

5a43055

Minimize interface, add static quadrature rules library

64c9dc3

Add comments, increase readability

b9d0005

Update primal/CMakeLists.txt formatting

3b57151

Co-authored-by: Kenny Weiss <kennyweiss@users.noreply.github.com>

Added tests for CurvedPolygon interface.

1d61386

jcs15c force-pushed the feature/spainhour/bezier_quadrature branch from 3f5d834 to b8a0599 Compare June 22, 2022 23:50

Updated style/formatting, compilation optimization

4995da1

jcs15c force-pushed the feature/spainhour/bezier_quadrature branch from b8a0599 to 4995da1 Compare June 23, 2022 00:14

update release notes

6c02447

kennyweiss approved these changes Jun 23, 2022

View reviewed changes

kennyweiss requested a review from gunney1 June 23, 2022 15:46

jcs15c added 3 commits June 23, 2022 08:53

Updated comments

f4935d9

Added CurvedPolygon overload to evaluate_line_integral

b4bef28

Updated to r-value reference for lambda functions

0534920

Merge branch 'develop' into feature/spainhour/bezier_quadrature

b6b1b5b

publixsubfan approved these changes Jun 24, 2022

View reviewed changes

agcapps reviewed Jun 24, 2022

View reviewed changes

jcs15c added 3 commits June 24, 2022 09:00

Changed quadrature rule pointers to references

ddc9791

Changed quadrature rule pointers to references

4d0d9e1

Merge branch 'feature/spainhour/bezier_quadrature' of https://github.…

39d5cb7

…com/LLNL/axom into feature/spainhour/bezier_quadrature

agcapps reviewed Jun 24, 2022

View reviewed changes

Temporarily removed R-value references

702de76

agcapps approved these changes Jun 24, 2022

View reviewed changes

Add tests for line integrals on disconnected curves

de51f54

kennyweiss mentioned this pull request Jun 25, 2022

Task/config options doc #853

Merged

Added extra dependencies for Primal

8f75254

rhornung67 added this to the March 2022 Release milestone Jun 27, 2022

jcs15c added 2 commits June 27, 2022 16:54

Merge branch 'develop' into feature/spainhour/bezier_quadrature

218eac7

Merge changes from develop

1c3c99d

jcs15c merged commit 4acbe95 into develop Jun 28, 2022

jcs15c deleted the feature/spainhour/bezier_quadrature branch June 28, 2022 18:47

jcs15c restored the feature/spainhour/bezier_quadrature branch August 4, 2022 16:26

	std::function<double(Point2D)> integrand,
	std::function<double(Point2D)>&& integrand,

		* Regions Bounded by Rational Parametric Curves" by David Gunderman et al.
		*/

Conversation

jcs15c commented Jun 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kennyweiss left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kennyweiss left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

publixsubfan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agcapps Jun 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agcapps left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jcs15c commented Jun 21, 2022 •

edited

Loading

agcapps Jun 24, 2022 •

edited

Loading