LLNL / conduit

Simplified Data Exchange for HPC Simulations
https://software.llnl.gov/conduit/
Other
212 stars 65 forks source link

change `topology::unstructured::to_polygonal` to `to_polytopal`? #745

Closed xjrc closed 2 years ago

xjrc commented 3 years ago

The new name would more accurately describe what the function does, but it would require updating some downstream libraries. This decision will be important as it will also impact #642 and the final names used for the functions in this PR.

Tagging: @cyrush, @nselliott

cyrush commented 3 years ago

as a bridge, should we have both?

xjrc commented 3 years ago

I think my preference would be skipping the bridge because the code isn't used very many places (I think just my code and potentially @terryhaut 's code), but I'm fairly indifferent. Is there any way we can at least mark to_polygonal as deprecated and have it complain when called?

cyrush commented 3 years ago

@nselliott Q:

For the non mpi lib blueprint, there was only one method to_polygonal that supported both polygons and polyhedra.

For the mpi lib blueprint, there are both to_polygonal and to_polyhedral methods.

What are your thoughts about to_polytopal to unify both cases?

nselliott commented 3 years ago

@cyrush I have no problem with a name change, but perhaps there should be some differentiation between the naming of the non-MPI and MPI versions? They aren't really serial/parallel versions of the same thing.

cyrush commented 2 years ago

to_polygonal case for non-mpi lib is under mesh::topology::unstructured. With this, it is implied to operate on a mesh topology.

The mpi methods operate on the entire mesh.

mpi::mesh::to_polygonal for assumes the input mesh is a 2D structured mesh with adjsets (and amr level info ?). mpi::mesh::to_polyhedral for assumes the input mesh is a 3D structured mesh with adjsets (and amr level info ?).

I am a little confused that these methods don't actually use MPI to communicate?

I think that's a good thing, it signals that the adjsets construct provides enough info, but I also wonder that's just part of the current assumptions.

If we truly don't need to use MPI, we should think about moving these methods into blueprint::mesh

nselliott commented 2 years ago

They do use MPI, for example lines 448-459 in conduit_blueprint_mpi_mesh.cpp

cyrush commented 2 years ago

I see. I was confused by the interface. We need to change these to take a communicator and not assume MPI_COMM_WORLD