Conversation
…es gaps in to element ID space
…ing using intrinsic IDs
pshriwise
force-pushed
the
index-conversion
branch
from
80a24a1 to
3333fdf
Compare
The addition of a functor here allows MOAB EntityHandles to be converted to MeshIDs on-the-fly and avoids the allocation of an intermediate iterable of MeshIDs just for the purpose of establishing this mapping.
pshriwise
requested review from
Waqar-ukaea and
gonuke
and removed request for
gonuke
|
This got a little more involved than I'd bargained for, but in the end I'm alright with where it landed. I think this should be ready for a look when ppl have a chance. |
Waqar-ukaea
left a comment
Waqar-ukaea
left a comment
There was a problem hiding this comment.
Changes look pretty good to me. I just have some general comments mostly just for checking my own understanding of some of the new code before I am happy to approve.
There was a problem hiding this comment.
I'll give @gonuke a chance to review, since he likely has more insight on the MOAB side of things. But otherwise, I am happy to merge.
gonuke
left a comment
gonuke
left a comment
There was a problem hiding this comment.
Sorry for these little ticky-tack comments here, but I don't think I have time to fully review the actual meat of this PR.
include/xdg/mesh_manager_interface.h
Outdated
| { return vertex_id_map_.index_to_id(vertex_idx); } | ||
|
|
||
| //! \brief Return the index of a vertex given its ID in the mesh | ||
| virtual int vertex_index(MeshID vertex) const |
There was a problem hiding this comment.
should this return size_t to be consistent with vertex_id() that takes a size_t as input?
include/xdg/mesh_manager_interface.h
Outdated
| //! \brief Convert an element's index in the mesh to its ID | ||
| //! \param element The element ID | ||
| //! \return The index of the element in the mesh | ||
| virtual int element_index(MeshID element) const |
There was a problem hiding this comment.
same comment about returning size_t for round trip consistency
Still helpful and appreciated! I've updated the return types of those methods to |
|
@gonuke does everything you had time to look at seem okay at this point? |
gonuke
left a comment
gonuke
left a comment
There was a problem hiding this comment.
Happy to move this forward
|
Do we care about the failing test? |
Mmm yes we do, but I might address the root cause in a separate PR. There's a strange bug in libMesh in handling strings I've yet to look into. Trying a re-run of the test for now. |
3 participants
This is a possible solution to #185.
libMesh will take more work than is shown here, but for all of the cases I've used so far the element ID space has been contiguous for replicated, unrefined meshes, so it should be effective for the time being.
This will keep us moving on the OpenMC integration front so we can reliably identify an element's index from an ID regardless of the backend, but tbh I'm not crazy about it so I'm marking it as a draft.