-
Notifications
You must be signed in to change notification settings - Fork 12
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
Adds support for meshing in-vessel components #178
Adds support for meshing in-vessel components #178
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.
Thanks @tokamaster - this looks pretty good to me. I have made one minor request for future readability, but want to make sure @connoramoreno has a chance to chime in as well.
parastell/cubit_io.py
Outdated
@@ -205,3 +205,14 @@ def export_dagmc_cubit_native( | |||
# exports | |||
if delete_upon_export: | |||
cubit.cmd(f"delete mesh volume all propagate") | |||
|
|||
|
|||
def create_new_cubit_instance(): |
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 thin it would be helpful to inser this immediately after init_cubit()
so that folks can find it and associate it with that method more easily.
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.
Yeah this makes sense; I'll insert it after init_cubit().
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 contribution @tokamaster! I just have a couple small thoughts on naming, otherwise this looks good to me
parastell/invessel_build.py
Outdated
@@ -346,6 +346,33 @@ def export_cad_to_dagmc(self, dagmc_filename="dagmc", export_dir=""): | |||
|
|||
model.export_dagmc_h5m_file(filename=str(export_path)) | |||
|
|||
def export_invessel_component_mesh( |
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 this is part of the InVesselBuild
class, I don't think it's necessary to specify in the name that this method is for in-vessel components. We may want to shorten the name to something like export_mesh
for brevity and also to match the method name we use for the magnets.
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.
Yep I agree 'invessel' here is redundant. Yet, I think I would keep 'components' in the name of the method, since it meshes, and exports, specific components and not the whole radial build. Something like export_component_mesh
would be more appropriate in my opinion. A similar logic follows for generate_components
method in the InVesselBuild
class.
@@ -191,6 +191,27 @@ def export_invessel_build( | |||
dagmc_filename=dagmc_filename, export_dir=export_dir | |||
) | |||
|
|||
def export_invessel_component_mesh( |
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 we might want to match the name of this method to that of the corresponding class for consistency, i.e., export_invessel_build_mesh
.
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 would still keep 'component' in the method's name since it meshes specific components, defined as an argument, rather than the entire radial build. We could have another method for meshing the entire build, though.
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.
Just one small comment about the call to export_mesh_cubit
in the InVesselBuild.export_component_mesh
method.
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 looks good to me - thanks @tokamaster
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.
LGTM as well, thanks for this contribution @tokamaster!
This pull request introduces a method for generating a tetrahedral mesh of individual in-vessel components and exporting the mesh in .h5m format. The process involves the following steps:
When using the method, the names of the in-vessel components must match the names specified in the radial build dictionary. Also, the mesh size can be customized using the mesh_size argument. More information on mesh sizing functions can be found here.