Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reuse mesh when saving to VTX #2738

Merged
merged 56 commits into from
Jan 24, 2024
Merged

Reuse mesh when saving to VTX #2738

merged 56 commits into from
Jan 24, 2024

Conversation

massimiliano-leoni
Copy link
Contributor

@massimiliano-leoni massimiliano-leoni commented Aug 3, 2023

Similarly to Fides, VTX too supports saving the mesh only once and reusing it for all time steps. The reason this didn't work out-of-the-box when we implemented the corresponding functionality for Fides was that, while the mesh was only saved once and the actual finite element functions were saved many times -- which is correct -- the functions "vtkOriginalPointIds" and "vtkGhostType" were saved only once too as they are saved when saving a mesh. For some reason, ADIOS2 needs them to be saved for each time step, probably because it needs all functions to be saved at all time steps. See https://gitlab.kitware.com/vtk/vtk/-/issues/19033 for more details.

This pull request follows in the footsteps of the one that implemented mesh reuse for Fides but also fixes this issue by caching the data for "vtkOriginalPointIds" and "vtkGhostType" when the mesh is written and saving additional copies of these functions at each time step.

The main change is in VTXWriter::write where the code now checks if the policy is to always write the mesh [or if it's the first write], in which case it writes the mesh before writing the functions. In case the policy is to reuse the mesh, it only writes the necessary data [as discussed above] and the functions.

@jorgensd
Copy link
Sponsor Member

A few things that I think could be useful to consider:

  1. Python interface (we want it to be possible to control it from there)
  2. Single constructor. The engine (BP4) should be possible to control
  3. Does VTX support having meshes on a sub-set of time steps? i.e. say we have 10 time steps, but the mesh is constant from step 0-3, step 4-8 and step 9. If it is possible to then just store 3 meshes, the policy should be input to the write function rather than the constructor.
  4. Does VTX support the mesh topology changing? If not, can we just write the geometry and not topology to file for every rewrite?

Copy link
Sponsor Member

@jorgensd jorgensd left a comment

Choose a reason for hiding this comment

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

The only thing I'm missing from this PR is a test in C++ or Python on usage, as adding it with no testing in the library is a way of introducing bugs.

@massimiliano-leoni
Copy link
Contributor Author

@jorgensd I added a unit test.

Copy link
Member

@jhale jhale left a comment

Choose a reason for hiding this comment

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

Some small comments then I think this can be merged.

cpp/dolfinx/io/ADIOS2Writers.h Show resolved Hide resolved
cpp/test/io.cpp Show resolved Hide resolved
cpp/test/io.cpp Show resolved Hide resolved
Copy link
Sponsor Member

@jorgensd jorgensd left a comment

Choose a reason for hiding this comment

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

Added a test that checks if data is placed in appropriate steps with the VTXMeshPolicy.update and VTXMeshPolicy.reuse.
I guess we should add a note somewhere that reuse can only be opened in Paraview>=5.12

python/test/unit/io/test_adios2.py Outdated Show resolved Hide resolved
@massimiliano-leoni
Copy link
Contributor Author

Anything else that I can do for you guys? :)

@jorgensd jorgensd requested a review from jhale January 22, 2024 14:21
@jorgensd jorgensd added this pull request to the merge queue Jan 24, 2024
Merged via the queue into FEniCS:main with commit f2b3054 Jan 24, 2024
12 checks passed
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.

5 participants