-
Notifications
You must be signed in to change notification settings - Fork 16
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
Add method to convert DAGMC volume to STEP #203
Conversation
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.
This all looks pretty straightforward, but just one question out of ignorance that might simplify writing the STLs.
parastell/utils.py
Outdated
""" | ||
volume = dagmc_model.volumes_by_id[volume_id] | ||
|
||
num_tris = len(volume.triangle_handles) |
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.
Did you think about just writing these triangles (volume.triangle_handles
) into a single STL? I have no idea whether it would work, just curious?
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'm not sure what it would look like to use the triangle handles directly - I know that both mbconvert and something like vol.model.mb.write_file(stl_path, output_sets=[vol.handle])
fail to write volumes to stl, while writing surfaces to stl works just fine. I also have had better luck making these volumes in CadQuery starting from surfaces, though that might be more related to user error when I was first figuring things out.
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.
It might be necessary to make a new set that contains the volume triangles - the write method may want a "set" rather than a "range" (which is what volumes.triangle_handles
is)
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.
It may not be worth it - or we could make it an issue for an optional simplification in the future.
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.
Since I can't write the triangles in the volume directly to STL with the same method that works with surfaces, (seems to be some problem with MOAB) I think anything we come up with will be about the same level of complexity as writing each surface in a volume to STL. I'm inclined to leave it as is.
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.
Right, I think the issue is that the volume entity set itself doesn't contain any triangles. See the image on the data model below from one of @gonuke's slide decks.
The VTK writer in MOAB seems to traverse any child sets of the output_sets
argument to collect elements -- probably because the VTK format is more generic, accommodating many different element types/dimensions so it does more work. In contrast, STL files (generally) contain single groups of triangles (this is how an STL will be read into MOAB, for instance, so I think MOAB is expecting to find the elements in the set(s) passed to the write_file
call. This doesn't mean the STL writer in MOAB shouldn't be traversing the child sets like the VTK reader, just an explanation as to why it may not based on the allowed contents of these two different file types.
So, rather than passing the volume handle, collecting the handles of the volume's surface sets (which do contain triangle elements) would do the trick. It would look something like:
model.mb.write_file('vol.stl', output_sets=[s.handle for s in volume.surfaces])
I gave this a try using one of the .h5m
files in the OpenMC DAGMC regression tests and it seemed to work alright.
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.
Here's a simple example:
import dagmc
dm = dagmc.DAGModel('dagmc.h5m')
vol = dm.volumes_by_id[2]
print('writing to vtk...')
vol.model.mb.write_file('vol2.vtk',output_sets=[vol.handle])
print('writing to stl...')
vol.model.mb.write_file('vol2.stl',output_sets=[vol.handle])
and the output
writing to vtk...
writing to stl...
Writer with name STL for file vol2.stl using extension stl (file type "(null)") was unsuccessful
MOAB ERROR: --------------------- Error Message ------------------------------------
MOAB ERROR: No triangles to write!
MOAB ERROR: write_file() line 96 in src/io/WriteSTL.cpp
Using default writer WriteHDF5 for file vol2.stl
The vtk file is valid, I can open it up in paraview just fine. It does write a file for the stl, but writes to HDF5 with an STL suffix.
Attempting to write the triangle handles with:
vol.model.mb.write_file('vol2.stl', output_sets=vol.triangle_handles)
give this error
Cell In[2], line 1
----> 1 vol.model.mb.write_file('vol2.stl', output_sets=vol.triangle_handles)
File pymoab/core.pyx:153, in pymoab.core.Core.write_file()
OSError: Only EntitySets should be passed to write file.
Which is strange since I believe vol.triangle_handles
is a range and the docstring for write_file
seems to indicate ranges are valid for output_sets
.
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.
Whoops, didn't see your comment here @gonuke. I think output_sets
does need to be either an EntitySet
or Iterable of EntitySet
's as opposed to element handles, but it's been a while and I could be wrong here.
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.
Ah I got it vol.model.mb.write_file('vol2.stl', output_sets=vol._get_triangle_sets())
works. output_sets
needs a range of EntitySet
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 need to refresh PR pages more often I missed all the comments in the past half hour and was just blissfully writing my own :)
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 think the simplification was worth it - now a few final touch ups.
parastell/utils.py
Outdated
with tempfile.NamedTemporaryFile(delete=True, suffix=".stl") as temp_file: | ||
stl_path = temp_file.name | ||
volume.model.mb.write_file( | ||
stl_path, output_sets=volume._get_triangle_sets() |
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 think I like this suggestion from @pshriwise better since it doesn't rely on the nominally private interface
stl_path, output_sets=volume._get_triangle_sets() | |
stl_path, output_sets=[s.handle for s in volume.surfaces] |
stl_path, output_sets=volume._get_triangle_sets() | ||
) | ||
cq_solid = stl_to_cq_solid(stl_path, tolerance) | ||
num_faces = len(cq_solid.Faces()) |
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.
Since it is only used here, can we move the definition of num_tris
to here?
tests/test_utils.py
Outdated
|
||
with tempfile.NamedTemporaryFile(delete=True, suffix=".stl") as temp_file: | ||
stl_path = temp_file.name | ||
vol.model.mb.write_file(stl_path, output_sets=vol._get_triangle_sets()) |
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.
as above
vol.model.mb.write_file(stl_path, output_sets=vol._get_triangle_sets()) | |
dagmc_model.mb.write_file(stl_path, output_sets=[s.handle for s in vol.surfaces]) |
parastell/utils.py
Outdated
|
||
with tempfile.NamedTemporaryFile(delete=True, suffix=".stl") as temp_file: | ||
stl_path = temp_file.name | ||
volume.model.mb.write_file( |
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.
Since you already have the model, this may be better
volume.model.mb.write_file( | |
dagmc_model.mb.write_file( |
tests/test_utils.py
Outdated
num_faces = len(cq_solid.Faces()) | ||
|
||
assert num_faces == num_tris | ||
assert math.isclose(dagmc_volume_volume, cq_solid_volume) |
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.
either pytest or numpy have a way to do this instead of math
?
tests/test_utils.py
Outdated
dag_model = dagmc.DAGModel("files_for_tests/one_cube.h5m") | ||
vol = dag_model.volumes_by_id[vol_id] | ||
dagmc_volume_volume = vol.volume | ||
num_tris = len(vol.triangle_handles) |
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.
Move this close to the definition of num_faces
since they are used together
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 this addition @Edgar-21
Closes #202
Two methods are added to
parastell.utils
, one which reads STL files defining the surfaces of the volume to be converted and returns a CadQuery Solid defined by those surfaces, and another than streamlines the process by taking a PyDAGMC DAGModel object and a volume ID and writing the corresponding CadQuery Solid to STEP.