Reconstruction Kernel Fusion 1: Convert to structs #371
Conversation
bcaddy
force-pushed
the
dev-pcmFusion
branch
2 times, most recently
from
237fad2 to
be2c704
Compare
bcaddy
changed the title
There was a problem hiding this comment.
Okay, I had a look through everything, but focused on basic_structs.h and reconstruction_internals.h. I think this looks really great! I find that these changes improve readability quite a bit. I mostly had comments about naming/comments to improve clarity. You can take them or leave them!
This change makes it clearer that the file contains some of the internal workings of the reconstructors and not the reconstructors themselves. It also clears the way for a new reconstruction.h file that can contain the controller function for the future device function version of the reconstructors.
It's not used anywhere yet but will be used in the fused reconstruction
I moved the reconstruction::Primitive struct into the hydro utilities. Along with that I added a struct for conserved variables and a small struct for vector quantities that just has members x, y, and z to make storage and movement of vectors easier. I also made the primitive and conserved structs use that vector struct for their vector quantites. Once that was done I fixed all the references to reconstructon::Primitive to point at the new location/version of the struct, updated it's usage, and updated all the relevant tests.
Renamed to gas_energy_specific to make usage clearer
Refactor the Exact Riemann solver to use the new InterfaceState struct in prep for reconstruction refactor
Refactor the Roe Riemann solver to use the new InterfaceState struct in prep for reconstruction refactor
Refactor the HLL Riemann solver to use the new InterfaceState struct in prep for reconstruction refactor
Refactor the HLLC Riemann solver to use the new InterfaceState struct in prep for reconstruction refactor
This constructor maens that when the struct is initialized in tests there is no ABI dependence and instead the internals can be easily changed without effecting the interface. Also, fixed some tests that had bugs due to their ABI dependency.
Refactor the HLLC Riemann solver to use the new `InterfaceState` struct in prep for reconstruction refactor. This required a bit more work as there than the other Riemann solvers since I'm replacing the existing mhd::internal::State struct rather than replacing a bunch of scalar variables. To do this I also added a constructor to `reconstruction::InterfaceState` to eliminate the previous ABI dependency on `mhd::internal::State`
This was to avoid a circular dependency between the mhd and hydro util files. Now this fundemental structs are on their own and can be included as needed Fix typo in Conserved struct
Renamed to indicate that they are the specific quantities
Some variables were accidentally misnamed and there was a typo in part of VL_3D related to the exact solver. Both issues have been fixed
Now it's consistent with Compute_Slope
This was done to avoid confusing with std::vector and DeviceVector
This version supports indexing rather than just accessing the xyz members directly. Matthew Abruzzo provided the new version of the struct, Robert Caddy added doxygen comments and implemented it in Cholla. Co-authored-by: Matthew Abruzzo <matthewabruzzo@gmail.com>
|
I'm happy to make this change. I do want to point out that except for the single Riemann solver that uses the bulk quantities, in every other place used it is the specific version of the gas energy and scalar which is why I chose those names. Do you still want me to change it? |
|
Yes, we had a discussion about it and everyone agreed that unless it is actually true everywhere, it's better to leave the name more ambiguous and comment every time the variable is first used. |
|
As I believe Helena pointed out, it is also true that in the future there may be additional versions of scalars where it makes more sense not to use the specific version, but we don't necessarily to want to add a whole additional variable to the struct, when part of the point is to streamline/abstract things. |
|
Fair enough, I'll remove it. |
|
Done, see PR #397 |
Summary
The goal of this PR is a first step in preparing to fuse the reconstruction and Riemann solver kernels. To do this it converts many variables in both the reconstructions and the Riemann solvers into structs. Most of these structs were already present in the
reconstructionandmhdnamespaces but they have been generalized and moved into thehydro_utilitiesnamespace in thebasic_structs.hfile and thereconstructionnamespace inreconstruction.h.Given the number of changes and the new structure of data I would like everyone to at least glance through the contents of
basic_structs.handreconstruction.hto see if it looks good to them since I plan on using those structs throughout the reconstructors.Structs
hydro_utilitiies::PrimitiveContains all the primitive variables and the specific versions of the gas energy and scalars. Included a default and non-default constructor, mostly the non-default constructor is for tests since the previous usage of this struct in tests had ABI dependencies; the new constructor provides a more flexible API to shield the internal arrangement of the struct.
hydro_utilitiies::ConservedContains all the conserved variables and the non-specific versions of the gas energy and scalars. Also has a constructor for the reasons listed above.
hydro_utilitiies::VectorThis just has three members,
x,y, andz, and is intended to serve as an easy way to move vector quantities around. I don't love the name since it sort of conflicts withstd::vectorbut they're in different namespaces and this one stores actual mathematical vectors. This struct is used in both thePrimitiveandConservedstructs to store their vector quantities. I would like some feedback on naming, usefulness, etc of this struct.reconstruction::InterfaceStateContains all the variables used in the Riemann solver interface states. It's basically all the primitive and conserved variables together in one struct.
Renamed
reconstruction.htoreconstruction_internals.hIn the final version I want a file called
reconstruction.hthat only has the Riemann solver facing code and a separate one that has all the code that is shared between the reconstructors. This is why I renamedreconstruction.hand created a newreconstruction.hthat contains only theInterfaceStatestruct at the moment.