Skip to content

Irina/hdf5 c - #53

Open
ipdemes wants to merge 39 commits into
masterfrom
irina/hdf5-C
Open

Irina/hdf5 c#53
ipdemes wants to merge 39 commits into
masterfrom
irina/hdf5-C

Conversation

@ipdemes

@ipdemes ipdemes commented Feb 25, 2019

Copy link
Copy Markdown

Creating a new pull request for the work @dmringo was doing

Irina and others added 30 commits October 2, 2018 13:43
re work on connectivity information
This compiles OK, needs testing
- Use FleCSI convention of `_u` for incomplete templated things
- Need to include MPI to use MPI
- Expect Native Int for connectivity data
- Example of getting names (unreliable) for error messages
  - initial tests indicate that Datatypes don't always have retrievable names
@ipdemes
ipdemes requested a review from charest February 25, 2019 21:52
@charest

charest commented Feb 25, 2019

Copy link
Copy Markdown
Collaborator

Can you please squash this into fewer commits.

// execute the mpi task to partition the mesh
flecsi_execute_mpi_task(partition_mesh, flecsi_sp::burton, mesh_filename,
max_entries);
if(extension == "exo" || extension == "g") {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ipdemes @dmringo I'm looking at extending our runtime selection of type of mesh to parse, in particular looking towards X3D format meshes. Is having separate tasks for each format the approach you two have decided upon? I had stubbed out some code that pushed the if check down into the task (the singular partition_mesh task), but this involved casting the mesh definition object (e.g. exodus_definition_t or x3d_definition_t) to the common base class mesh_definition_u for some operations. That was part of the motivation of PR 553 in FleCSI

If we want to go the route of separate tasks for partitioning and initializing, that is fine. It cleans up some things, but does require, perhaps, a bit of code duplication. In general I'm against that sort of thing, but in this particular case I don't really have strong feelings either way.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cmsquared : do you have your work in a branch somewhere? So I can have a look?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately not. But the changes to the partition_mesh task looked something like this:

  // mesh definitions
  using mesh_definition_t = flecsi::topology::mesh_definition_u<num_dims>;
  using exodus_definition_t = flecsi_sp::io::exodus_definition__<num_dims, real_t>;
  using x3d_definition_t = flecsi_sp::io::x3d_definition__<num_dims, real_t>;

  // load the mesh
  auto filename_string = filename.str();
  std::unique_ptr<mesh_definition_t> mesh_def;
  if (filename_string.find(".exo") != std::string::npos)
    mesh_def = std::make_unique<exodus_definition_t>(filename_string);
  else if (filename_string.find(".x3d") != std::string::npos)
    mesh_def = std::make_unique<x3d_definition_t>(filename_string);
  else
    clog_fatal("Burton specialization doesn't know how to read " <<
               filename_string);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cmsquared I'm looking at this again, and liked your approach, but ran into problems resolving the write method on the mesh definition (called near the end of partition_mesh) since it's not part of the abstract mesh_definition_u interface. Did you cast to the concrete implementation to solve this? Unless there are plans to add that method to the interface in FleCSI, that makes it seem like a partition_mesh task templated on the IO backend would be preferred.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants