AH rewrite: Add function to interpolate AH volume vars #6775
Conversation
wthrowe
left a comment
There was a problem hiding this comment.
My overall comment here is that this needs a lot more documentation, since I'm still not sure what the function is actually doing. I've given some code comments, but I may have more insight after the docs are updated.
| #include "ParallelAlgorithms/ApparentHorizonFinder/Storage.hpp" | ||
|
|
||
| template <size_t VolumeDim> | ||
| class Domain; |
There was a problem hiding this comment.
/// \cond around the forward declarations.
|
|
||
| namespace ah { | ||
| /*! | ||
| * \brief Interpolate volume data from any new elements to the target points |
There was a problem hiding this comment.
Expand on this documentation a bit. What are "new elements"? None of the const arguments contain any data, so what are the inputs and outputs?
| */ | ||
| template <typename Fr> | ||
| void interpolate_volume_data( | ||
| gsl::not_null<ah::Storage::Iteration<Fr>*> current_iteration_storage, |
|
|
||
| // Loop over each tensor | ||
| tmpl::for_each<ah::vars_to_interpolate_to_target<3, Fr>>([&](auto tag_v) { | ||
| using tag = tmpl::type_from<decltype(tag_v)>; |
There was a problem hiding this comment.
[optional] Use an explicit template parameter on the lambda. Also in a few other places.
| // Temporarily disable FPEs | ||
| const ScopedFpeState fpe_state{false}; | ||
| interpolated_vars.initialize( | ||
| expected_num_points, std::numeric_limits<double>::signaling_NaN()); |
There was a problem hiding this comment.
Are the nans being tested for somewhere else? I.e., is there a reason to override the default initialization behavior in non-debug builds?
wthrowe
left a comment
There was a problem hiding this comment.
You can squash, including the new docs.
| * `ah::Tags::CurrentTime`. The volume data that is received from Elements is | ||
| * stored in \p all_volume_variables and information on which elements have | ||
| * already interpolated their volume data is stored in | ||
| * \p current_iteration_storage. |
There was a problem hiding this comment.
This still doesn't actually tell me what this function does. How about something along the lines of "For each new element, the vars_to_interpolate_to_target in \p all_volume_variables are computed from its source_vars member, and the results are interpolated and stored in \p current_iteration_storage.".
|
Done! |
Proposed changes
Ninth PR in horizon finder changes. This adds a function that runs every time a new element has sent and added its data to the horizon finder. It tries to interpolate the volume data from any new elements received to the trial surface points.
Upgrade instructions
Code review checklist
make docto generate the documentation locally intoBUILD_DIR/docs/html.Then open
index.html.code review guide.
bugfixornew featureif appropriate.Further comments