FE Quadrature Phase 01 abstract contract - #593
Conversation
updating fork repo with upstream head
update main to upstream branch
|
@ktbolt @michelebucelli let me know what you think of the abstract class contract for quadrature. I believe the public API for this contract is minimal and clean while ensuring that explicit Quadrature rules that are created can be validated. As always, happy to have more design input here and will incorporate changes pretty quickly so we can begin making a concrete QuadratureRule class. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #593 +/- ##
==========================================
+ Coverage 72.54% 72.82% +0.27%
==========================================
Files 252 255 +3
Lines 39032 39511 +479
Branches 6678 6710 +32
==========================================
+ Hits 28317 28775 +458
- Misses 10480 10500 +20
- Partials 235 236 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
michelebucelli
left a comment
michelebucelli
left a comment
There was a problem hiding this comment.
Thank you @zasexton! I have left some comments. Some of them relate to the documentation, others to the interface itself.
In general, I am not sure I have understood correctly how the concrete rules are meant to be implemented (as you might see from some of my comments). In this respect, I think it would be useful to include in this PR at least one concrete rule (I assume you have implemented some, at least for the sake of testing the infrastructure), which would serve as a demonstration of how to use the abstract framework. I think this would make review a bit easier.
| COMMAND "${CMAKE_COMMAND}" -E remove_directory | ||
| "${SV_SOURCE_DIR}/../Documentation/build" | ||
| COMMAND "${DOXYGEN_EXECUTABLE}" "${SV_BINARY_DIR}/Doxyfile" | ||
| WORKING_DIRECTORY "${SV_SOURCE_DIR}/.." |
There was a problem hiding this comment.
If I understand this right, the purpose of this is to fix the make doc recipe (whereas as of now the documentation would have to be built by calling doxygen manually from the repository root folder). Is this correct?
If so, is the documentation now generated into the build folder, or into the source folder as in the current main branch?
| file(GLOB SOLVER_FE_QUADRATURE_SRCS CONFIGURE_DEPENDS | ||
| FE/Quadrature/*.cpp | ||
| FE/Quadrature/*.h | ||
| ) | ||
|
|
||
| list(APPEND CSRCS | ||
| ${SOLVER_CORE_SRCS} | ||
| ${SOLVER_FE_COMMON_SRCS} | ||
| ${SOLVER_FE_BASIS_SRCS} | ||
| ${SOLVER_FE_MATH_SRCS} | ||
| ${SOLVER_FE_QUADRATURE_SRCS} | ||
| ) |
There was a problem hiding this comment.
Wouldn't it be better to have separate CMakeLists.txt files in the individual subfolders, and to build separate binaries that are then linked into the executable? The main CMakeLists.txt would then recurse into subfolders, instead of listing all the files.
In my opinion this would encourage the clean separation of the different modules, and it might be a good idea to start implementing it at least for the newly refactored parts of the library.
There was a problem hiding this comment.
@zasexton I agree with @michelebucelli; better to use CMake to manage sub-directories.
| * This header defines the consumer-facing representation of a finite-element | ||
| * quadrature rule. Rule construction and validation are implemented separately | ||
| * so consumers depend only on the stable query interface. |
There was a problem hiding this comment.
Minor: I would drop "finite-element" here, since the concept of a quadrature formula is independent of what one does with it (generic integrals or FE integrals).
| * The module does not choose the exactness required by an equation term, apply | ||
| * reduced-integration policy, select a basis, own mesh storage, embed or orient | ||
| * a face in its parent element, or map a reference integral into physical | ||
| * space. Those operations require solver, basis, mesh, and geometry context and | ||
| * remain at the caller's integration boundary. |
There was a problem hiding this comment.
In the same spirit as my previous comment about mentioning finite elements, I think this paragraph addresses concerns that are somewhat unnatural in the context of quadrature (or at least should be), and so I feel that it might risk creating confusion in the reader, rather than clarifying. I suggest rephrasing as follows:
This module is not concerned with selecting the quadrature rule based on the requirement of any specific application (e.g. on what functions must be integrated exactly). Furthermore, this module only deals with quadrature rules on reference cells, not on transformed cells. Those concerns are left to the consumers of this module.
More in general, I appreciate (a lot!) the level of detail in this documentation. However, in some places I feel that it uses jargon that is appropriate in a software engineering context but might be obscure to people without a structured software engineering background. Since I feel that many of the contributors to svmp might have a different background (e.g. applied mathematics or biomedical engineering), I think it might be beneficial to shift to a slightly more concrete and practical language. I will try to give some concrete suggestions in this direction in my other comments.
(The jargon issue might also be addressed by providing some sort of glossary, but I feel it might make things harder rather than easier to read).
There was a problem hiding this comment.
@zasexton I must admit that I am not very familiar with some of the terminology you use and have not ever seen them used in the CAD, FEA, robotics, etc. companies I've worked in. Like @michelebucelli says these terms will not be meaningful to the typical developer.
I am not quite on board with the first 100 lines of a file being comments; maybe this should be in a separate document.
I've seen criticism of code that needs extensive commenting; perhaps should be more expressive, clean and simple.
| * @ref svmp::FE::quadrature::QuadPoint "QuadPoint" and the const query surface | ||
| * of @ref svmp::FE::quadrature::QuadratureRule "QuadratureRule" form the public | ||
| * API. Integration consumers read rule metadata and ordered samples; they do | ||
| * not derive new rules or modify rule storage. | ||
| * | ||
| * Complete-data construction, reference-cell traits, point-containment checks, | ||
| * exact weight summation, concrete generators, caches, and rule-selection | ||
| * facilities are module implementation details. |
There was a problem hiding this comment.
In the vein of my previous comment, I suggest rephrasing as follows (or similar):
The public API exposes the ordered weight-point pairs from the quadrature rule through the methods point, weight, points and weights of the QuadratureRule class. Additionally, a number of methods are exposed to access metadata about the rule such as the number of points or the degree of exactness. Other modules using quadrature formulas should exclusively rely on this information, and not on specific details of the concrete quadrature formulas being used.
(inserting Doxygen @refs appropriately so that the listed methods are linked back to their documentation).
| constexpr std::optional<ReferenceCellTraits> reference_cell_traits( | ||
| svmp::CellFamily family) noexcept | ||
| { | ||
| switch (family) { | ||
| case svmp::CellFamily::Point: | ||
| return ReferenceCellTraits{0, 1.0}; | ||
| case svmp::CellFamily::Line: | ||
| return ReferenceCellTraits{1, 2.0}; | ||
| case svmp::CellFamily::Triangle: | ||
| return ReferenceCellTraits{2, 0.5}; | ||
| case svmp::CellFamily::Quad: | ||
| return ReferenceCellTraits{2, 4.0}; | ||
| case svmp::CellFamily::Tetra: | ||
| return ReferenceCellTraits{3, 1.0 / 6.0}; | ||
| case svmp::CellFamily::Hex: | ||
| return ReferenceCellTraits{3, 8.0}; | ||
| case svmp::CellFamily::Wedge: | ||
| return ReferenceCellTraits{3, 1.0}; | ||
| default: | ||
| return std::nullopt; | ||
| } | ||
| } |
There was a problem hiding this comment.
Is there any legitimate situation where this should be called against an unsupported reference cell? If not, I would suggest throwing an exception in the default case, and dropping the std::optional altogether.
| case svmp::CellFamily::Point: | ||
| is_contained = true; | ||
| break; |
There was a problem hiding this comment.
Shouldn't this return true only if the provided point is coincident with the reference cell point (which I assume to be the origin [0, 0, 0])?
| if (polynomial_exactness < 0) { | ||
| return {"polynomial exactness must be non-negative"}; | ||
| } |
There was a problem hiding this comment.
Wouldn't this constraint be better represented by defining the polynomial exactness to be an unsigned int?
| message.append(" at sample "); | ||
| message.append(std::to_string(result.sample)); |
There was a problem hiding this comment.
What is the meaning of "sample" in this context? Does it refer to the index of the quadrature point? In that case, I think making more explicit reference to the quadrature point index might be more easy to interpret, and slightly more in line with standard nomenclature.
| QuadratureRule::QuadratureRule(svmp::CellFamily family, RuleData data) | ||
| : QuadratureRule(validate(family, std::move(data))) | ||
| { | ||
| } | ||
|
|
||
| QuadratureRule::QuadratureRule(ValidatedState state) | ||
| : cell_family_(state.cell_family), | ||
| dimension_(state.dimension), | ||
| polynomial_exactness_(state.polynomial_exactness), | ||
| reference_cell_measure_(state.reference_cell_measure), | ||
| points_(std::move(state.points)), | ||
| weights_(std::move(state.weights)) | ||
| { | ||
| } | ||
|
|
||
| QuadratureRule::ValidatedState QuadratureRule::validate( | ||
| svmp::CellFamily family, | ||
| RuleData data) | ||
| { | ||
| const auto traits = reference_cell_traits(family); | ||
| if (!traits) { | ||
| svmp::raise<InvalidArgumentException>( | ||
| validation_failure_message( | ||
| {"unsupported reference-cell family"})); | ||
| } | ||
|
|
||
| const auto validation = validate_rule_data( | ||
| family, | ||
| *traits, | ||
| data.polynomial_exactness, | ||
| data.points, | ||
| data.weights); | ||
| if (!validation.valid()) { | ||
| svmp::raise<InvalidArgumentException>( | ||
| validation_failure_message(validation)); | ||
| } | ||
|
|
||
| return { | ||
| family, | ||
| traits->dimension, | ||
| data.polynomial_exactness, | ||
| traits->reference_cell_measure, | ||
| std::move(data.points), | ||
| std::move(data.weights), | ||
| }; | ||
| } |
There was a problem hiding this comment.
This seems a bit more complicated than necessary to me. Wouldn't it be possible to merge all three methods (constructor from cell family and RuleData, constructor from ValidatedState and validate) into the first constructor only.
Unless there's some use cases that I am missing?
|
Thanks for these comments @michelebucelli I'll review these today and make appropriate changes. Overall there is a good bit of important design feedback here that will help me to improve this base abstraction (which is the main purpose of me posting this draft PR). Thank you! |
|
|
||
| constexpr double construction_validation_tolerance = 1.0e-12; | ||
| constexpr double coordinate_validation_tolerance = 1.0e-12; | ||
| constexpr double moment_validation_tolerance = 1.0e-12; |
| return ReferenceCellTraits{3, 8.0}; | ||
| case svmp::CellFamily::Wedge: | ||
| return ReferenceCellTraits{3, 3, 1.0}; | ||
| return ReferenceCellTraits{3, 1.0}; |
3 participants
Summary
This pull request establishes Phase 01 of the finite-element Quadrature module.
It defines the immutable reference-rule contract that concrete rule generation,
selection, and consumer migration can build on in later phases.
Related to #580.
The change:
QuadratureRuleinterface for canonicalreference cells.
reference-cell measure, and const sample access.
and immutable query interface.
the FE documentation hierarchy.
Design
Immutable rule contract
QuadratureRuleowns one complete set of ordered reference points and weights.Successful construction establishes the invariant, after which consumers use
only const queries. Rule objects are non-copyable and non-movable and are
intended to be created once and shared through const ownership.
Concrete providers construct a protected
RuleDatapayload containing:The base class derives dimension and reference-cell measure from
CellFamily,preventing providers from supplying redundant topology metadata.
Supported reference cells
The Phase 01 contract covers:
Pyramid, polygon, polyhedron, and unknown families remain explicitly
unsupported.
Construction validation
Construction rejects:
validation tolerance.
Duplicate points and zero or negative individual weights remain admissible when
all other invariants hold.
The weight sum is evaluated exactly from the stored binary64 values using a
fixed-storage accumulator. Validation is therefore independent of point/weight
ordering, cancellation, intermediate floating-point overflow, and the host
long doublerepresentation. Compile-time checks document and enforce thebinary64 value model and object layout required by that accumulator; this
will be important for cross-platform compile time validation.
Phase 01 regression tests
The Phase 01 tests are intentionally limited to the new Quadrature
infrastructure. They verify:
QuadPointrepresentation.QuadratureRulequery interface.containment.
reordered inputs, and large rules.
These tests do not exercise existing quadrature tables, solver-specific selection
paths, or integration consumers.
Supporting changes
QuadratureTypeenumeration from the sharedFE vocabulary.
FE/Quadraturesources to the solver target.before documentation generation, ensuring relative paths resolve
consistently.
Scope and non-goals
This pull request intentionally does not:
mapping.
Validation performed
run_all_unit_testsandsvmultiphysics.