Add DirectionId and DirectionIdMap #5621
Conversation
src/Domain/Structure/DirectionId.hpp
Outdated
| #include "Domain/Structure/Direction.hpp" | ||
| #include "Domain/Structure/ElementId.hpp" | ||
|
|
||
| namespace PUP { |
src/Domain/Structure/DirectionId.cpp
Outdated
| return h; | ||
| } | ||
|
|
||
| // clang-tidy: do not modify namespace std |
There was a problem hiding this comment.
can we NOLINT(SPECIFIC_ERROR) here?
| template <size_t Dim, typename T> | ||
| class DirectionIdMap | ||
| : public FixedHashMap<maximum_number_of_neighbors(Dim), DirectionId<Dim>, T, | ||
| boost::hash<DirectionId<Dim>>> { |
There was a problem hiding this comment.
Why not just leave the hasher defaulted? You added a std::hash for DirectionId
src/Domain/Structure/DirectionId.hpp
Outdated
| /// \endcond | ||
|
|
||
| template <size_t VolumeDim> | ||
| struct DirectionId { |
There was a problem hiding this comment.
Two thoughts: (1) DirectionId sounds like the ID for a direction, which made me wonder why we need an ID for it until I realized it stands for "Direction and neighbor ID". Related: (2) this same type is defined as MortarId in MortarHelpers.hpp. So maybe name it MortarId? It's not always used to identify DG mortars but IMO that's fine. If you don't like it maybe we can give it a thought on the Monday call.
There was a problem hiding this comment.
Just as a note in case I can't make the meeting for some unexpected reason, we used to have this data structure and called it DirectionalElementId.
I don't love the subcell code using MortarId since there are no mortars, really. Most of the places that would use it are actually indexing volume data. I worry that the subcell code is already too complicated with my best efforts to use clear names, so using a name like MortarId for indexing volume data, volume fluxes, neighbor RDMP data, etc. would make it extra confusing :(
There was a problem hiding this comment.
DirectionalElementId sounds good enough to me. We could alias using MortarId = DirectionalElementId in the dg namespace (not requesting this change). Why was the data structure removed (I don't remember it).
There was a problem hiding this comment.
Sounds good to both! My memory is that at some point we were only using it in a couple places and figured a std::pair was easier than having an extra class (turns out whatever group of us made that decision was wrong :P)
There was a problem hiding this comment.
I won't be able to make the meeting tomorrow. I'll do whatever the consensus is, but I'll suggest DirectionalId as that is shorter. I presume you want the same name change for DirectionIdMap as well.
| @@ -16,7 +16,7 @@ class er; | |||
| /// \endcond | |||
|
|
|||
| template <size_t VolumeDim> | |||
| struct DirectionId { | |||
| struct DirectionalId { | |||
There was a problem hiding this comment.
sorry I missed this on my last review. Could you document this a bit so it shows up in doxygen?
Use it instead of a std::pair<Direction<Dim>, ElementId<Dim>>.
Use it to when appropriate in subcell code. It will also be useful in AMR code.
|
ignoring test timeout |
Proposed changes
These are used throughout the subcell code, and I also want to use them in some AMR code.
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