Enhance CurvedPolygon so WN memoization works with sampling shaper #1672
Conversation
jcs15c
changed the title
primal::CurvedPolygon so WN memoization works with sampling shaper
jcs15c
added
enhancement
…ure/spainhour/sampling_shaper_memoization
kennyweiss
left a comment
kennyweiss
left a comment
There was a problem hiding this comment.
Looks reasonable so far.
Lingering questions for this PR include:
-
Many operations on a CurvedPolygon are forbidden at compile time (e.g. split and reverseOrientation) to ensure that the GWN cache is not made invalid. Is this the right approach? Or should the methods be allowed after the cache is cleared, and a warning message displayed to inform the user that this has happened?
- Seems reasonable to start by disallowing these operations (as long as this is documented) since invalidating the cache can have severe performance costs. If we need to allow this in the future, we can add support for it.
-
Some template boilerplate is required in CurvedPolygon.hpp to deduce the numeric type from the curve type. Is this implemented in the right way? Should those template methods be in a detail file?
- I'd leave it as is since it's reasonably self contained. Perhaps move it inside an anonymous namespace or a
detailnamespace (within the same file)?
- I'd leave it as is since it's reasonably self contained. Perhaps move it inside an anonymous namespace or a
…/spainhour/sampling_shaper_memoization
…/spainhour/sampling_shaper_memoization
jcs15c
changed the base branch from
develop
to
feature/spainhour/mfemless_gauss_legendre
…://github.com/LLNL/axom into feature/spainhour/sampling_shaper_memoization
…/spainhour/sampling_shaper_memoization
…/spainhour/sampling_shaper_memoization
kennyweiss
left a comment
kennyweiss
left a comment
There was a problem hiding this comment.
Thanks @jcs15c -- really nice improvement!
| template <typename Lambda, typename T, int NDIMS> | ||
| double evaluate_vector_line_integral(const axom::Array<primal::NURBSCurve<T, NDIMS>>& narray, | ||
| template <typename Lambda, typename CurveType> | ||
| double evaluate_vector_line_integral(const axom::Array<CurveType>& carray, |
There was a problem hiding this comment.
Possible generalization for the future -- perhaps the exposed API should use axom::ArrayView instead of axom::Array ?
There was a problem hiding this comment.
I think this is a good idea, as we discussed earlier, but I'm also running into a case where it's useful to template the return type of the lambda function so that you can perform integrals on vector-type functions. Since primal::Vector<T, NDIMS> already support addition and scalar multiplication, it's a pretty straightforward addition, and would be really helpful for computing things like the centroid of geometric objects, since it reduces the overhead of evaluating the function for a single input double. I think I'm going to put both ideas in a new feature branch.
Thanks for adding the performance improvement. Could you please also adding the performance change for a Release config? |
kennyweiss
requested review from
gunney1 and
rhornung67
and removed request for
bgunnar5
| for(int i = 0; i < beziers.size(); ++i) | ||
| { | ||
| total_integral += | ||
| detail::evaluate_scalar_line_integral_component(beziers[i], scalar_integrand, npts); |
There was a problem hiding this comment.
It may be good to add std::forward<Lambda>(scalar_integrand) here and in other places like this.
|
|
||
| Point<T, 2> m_initPoint, m_endPoint; | ||
|
|
||
| mutable axom::Array<std::map<std::pair<int, int>, BezierCurveData<T>>> m_bezierSubdivisionMaps; |
There was a problem hiding this comment.
Is some other object hanging onto a const reference or pointer to this cache, making it necessary to mark getSubdivisionData const, leading to this mutable?
There was a problem hiding this comment.
I think the idea is that this allows you to pass cache objects by const reference between functions, and use the same cache for each whiles still dynamically updating it. In a practical sense, calling getSubdivisionData is the primary purpose of these cache objects, so if it wasn't const, you couldn't realistically use const objects, but I don't know if that's especially satisfying. For the most part, this class was set up by pattern matching the design of the older MemoizedSectorAreaWeight class, so I'm open to the possibility that what worked there might not be best here.
BradWhitlock
left a comment
BradWhitlock
left a comment
There was a problem hiding this comment.
I had a few small suggestions. Overall, looks good. I like how CurvedPolygon is more general now.
This PR will be merged after #1683.
Summary
In order that the SamplingShaper in quest is compatible with recent GWN accelerations via memoization, the
primal::CurvedPolygon<T, NDIMS>class is enhanced so that it is templated directly on curve type, i.e.primal::CurvedPolygon<CurveType>. Currently supported curve types areBezierCurve,NURBSCurve, and the newNURBSCurveGWNCache. BecauseCurvedPolygonis used in many of primal's operators, this requires many updates:in_curved_polygonhave been made compatible with all curve types.compute_moments.hppare currently only compatible withCurvedPolygon<BezierCurve<T, 2>>, since the methods within expect Bezier curves, and would require a non-trivial extension to work onNURBSCurveobjects via Bezier extraction.The tests for these methods are updated in their respective suites to ensure compatibility with the new template.
On the quest side of things, this PR makes the following changes related to SamplingShaper
MFEMReaderso that the default input for an MFEM mesh is aCurvedPolygon<NURBS>instead of Bezier.WindingNumberSamper.hppso that the sampler stores an array of "cached contours," i.e.CurvedPolygon<NURBSCurveGWNCache>instead of a view ofCurvedPolygon<BezierCurve>s.Performance Improvements
These changes reduce the time to complete the test
quest_shaping_driver_ex_heroic_roses_mfem_cpfrom 85.93 seconds on a debug build ofdevelopto 56.68 seconds (or from 12.67s to 6.91s on a release build) on the current branch. Accounting for the time to process the curves and construct/access the BVH (~31.31s debug/~4.47s release) this is about a 2x improvement.