-
Notifications
You must be signed in to change notification settings - Fork 592
Adding vtkhdf option to write vtk data #3252
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
Adding vtkhdf option to write vtk data #3252
Conversation
|
Looks really nice @shimwell! It will be a great option to have. I'll review as soon as I am able. |
|
@pshriwise just to mention we are working on adding for more VTK based file formats in addition to this VTKHDF. We are also looking at adding VTU. So we are building on this branch with another feature. We won't make a PR for the VTU work till this VTKHDF PR has been merged or rejected, but I just wanted to let you know so you can take that into account when reviewing. |
tests/regression_tests/unstructured_mesh/test_mesh_dagmc_tets.vtk
Outdated
Show resolved
Hide resolved
Co-authored-by: rherrero-pf <[email protected]>
|
Related PR to enable export flexibility #3290 |
This comment was marked as resolved.
This comment was marked as resolved.
|
@pshriwise I've solved the merge conflicts and i think this one is ready for review when you have time. |
|
Just resolved a small conflict. |
|
@pshriwise just wondering if you have had time to review this PR |
|
I have been tinkering with python 3.14 as I couldn't resist the option to run without the famous gil. I have got openmc working for the most parts and also produced a wheel. The one thing I could not get working was any functions that import VTK. It appears there is no pypi release for VTK yet. https://pypi.org/project/vtk/#files This brought me back to the PR I made in January where I provided an option to write vtkhdf files which does not need the VTK package. I think this PR is even more useful now as it offers the a route to get even more of OpenMC working with python 3.14 by avoiding the use of the VTK package. I'm also thinking we should do the dame sort of vtkhdf5 for all the meshes when writing VTK format files and then look to make hdf5 the default in follow up PRs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I started a review but started having larger questions...
The new format looks pretty lightweight -- not bad to implement manually which is nice. I'm a little hesitant to take on maintaining writing these files manually when the vtkHDFWriter exists. Can you elaborate on this situation a bit?
We have this issue at the moment and can't import cadquery along with openmc due to slightly different vtk packages.
And I'll also apologize for how long it's taken for me to get back to this. Thanks for your patience @shimwell!
|
Can you elaborate on this situation a bit? We have this issue at the moment and can't import cadquery along with openmc due to slightly different vtk packages. My understanding is that Conda enforces packages are compiled with the same compiler / settings while pypi places no such restrictions on this. So installing cadquery, vtk, openmc both from pypi results in a binary incompatibility. Cadquery therefore makes and distributes its own version of vtk called cadquery-vtk that is built in a compatible way. They build and bundle VTK because the glibc version for the VTK package installed through pip is too new for many systems. There are related issues on this here. In summary it would be nice if we can write vtk format files but avoid using vtk. A we already have hdf5 as a dependency this provides a route to writing vtk format files without using vtk. I didn't know about the |
Co-authored-by: Patrick Shriwise <[email protected]>
Hi, VTK and VTKHDF maintainer here. I confirm that VTKHDF is designed to be usable without the need for using vtkHDFWriter and linking against VTK. Using hdf5 directly and following the VTKHDF spec is a perfectly valid approach and we take great care for retro compatibility and documentation of format change. I also confirm that VTK does not provide a 3.14 wheel yet but should soon start doing so, at least for VTK 9.6. hth, |
Indeed not, but its existence required me to ask :) @mwestphal thanks for taking the time to confirm the VTKHDF design intent and viability of a sustainable independent implementation for writing these files. I'm going to give this some more thought, but am leaning toward going forward with it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the refactor! It looks much cleaner to me. A few more comments to think over @shimwell.
Co-authored-by: Patrick Shriwise <[email protected]>
|
Thanks for the review, I have made the suggested changes. |
paulromano
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After thinking over this one, I'm on board despite my reluctance to support this code in OpenMC when equivalent functionality is availability out of vtk package directly. I've personally experienced the pain of dealing with the vtk Python package and would be keen on not having to bring it in as a dependency.
Looks like CI is failing due to the issue below:
As a VTK and VTKHDF maintainer, I'm glad to hear that because this is exactly for this usercase that VTKHDF was designed, to NOT require VTK to be needed and yet still being able to output ParaView/VTK compatible data. |
|
I took the liberty of making a few changes here @shimwell. Please let me know if they're alright with you.
I've got some changes lined up to enable hex element writing, but I'll save those for a follow-on PR after this goes in. |
|
Thanks Patrick. |
pshriwise
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making us aware of this capability and for the contribution, @shimwell! Once we support all meshes maybe we remove the need for the vtk module altogether. We'll see!
|
super, thanks for the improvements and approving. If there are no objections I can merge this in tomorrow. |
Description
As discussed in #3113 it would be great to have a method of writing VTKHDF files for mesh tally data.
This PR allows users to write .hdf file VTKHDF compatible files with the existing
UnstructuredMesh.write_data_to_vtk()method. This has a few advantages over the legacy vtk files we currently write.It would be particularly useful for those of us who can't make use of the vtk package in their environment. We have this issue at the moment and can't import cadquery along with openmc due to slightly different vtk packages. It is super useful that vtk is an optional dependency in openmc and not mandated. This use of h5py lets us avoid using the vtk package while still being able to write VTKHDF files
For interested people the VTKHDF roadmap is here, the ability to read these files (ending with hdf) was introduced in Paraview 5.13.0 release notes.
All credit goes to the Kitware developers and particularly @mwestphal who introduced me to this feature
Fixes # (issue)
partly addresses #3113
Checklist