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

Allow specifying dimension in Mesh.mesh_geometry #23

Merged
merged 4 commits into from
Dec 23, 2023

Conversation

paulromano
Copy link
Collaborator

This small change allows the dimension to be specified when meshing a geometry.

@akoen
Copy link
Collaborator

akoen commented Dec 18, 2023

Looks good. If the codebase is to be refactored to allow other export formats other than MOAB, I would maybe find a way to refactor out some of the MOAB-specific logic, like this.
https://github.com/Thea-Energy/stellarmesh/blob/026acbef2b23f6e936ceb7ffea9c543432dba5a0/src/stellarmesh/mesh.py#L114

I would also double check that this logic still holds (normals aren't changed) when we mesh up to 3d.
https://github.com/Thea-Energy/stellarmesh/blob/28a27175ce17213a1825abb0de584a5a56688fbc/src/stellarmesh/moab.py#L234

Finally, I would clarify/assert that mesh refinement will discard all volume mesh data
https://github.com/Thea-Energy/stellarmesh/blob/28a27175ce17213a1825abb0de584a5a56688fbc/src/stellarmesh/mesh.py#L192

These are all minor points, otherwise this small change should be good.

@paulromano
Copy link
Collaborator Author

If the codebase is to be refactored to allow other export formats other than MOAB, I would maybe find a way to refactor out some of the MOAB-specific logic

Yes, I'm planning on continuing down this path. Specifically, in moab.py we need better separation between things that are MOAB-specific and things that are DAGMC-specific. This will be in a future PR.

I would also double check that this logic still holds (normals aren't changed) when we mesh up to 3d

I wasn't sure exactly how to check this -- appreciate any pointers you have in that regard. However, from what I can tell, when you ask gmsh to generate a mesh up to dim=3, it starts with the lower dimensions which proceed exactly the same as if you had requested dim=2.

I would clarify/assert that mesh refinement will discard all volume mesh data

In fact, we're actually discarding volume mesh data as soon as MOABModel.make_from_mesh is called. I added a warning in there to at least note in the log that volume data was discarded.


Let me know if you think this is good to go!

@akoen
Copy link
Collaborator

akoen commented Dec 22, 2023

If the codebase is to be refactored to allow other export formats other than MOAB, I would maybe find a way to refactor out some of the MOAB-specific logic

Yes, I'm planning on continuing down this path. Specifically, in moab.py we need better separation between things that are MOAB-specific and things that are DAGMC-specific. This will be in a future PR.

Great!

I would also double check that this logic still holds (normals aren't changed) when we mesh up to 3d

I wasn't sure exactly how to check this -- appreciate any pointers you have in that regard. However, from what I can tell, when you ask gmsh to generate a mesh up to dim=3, it starts with the lower dimensions which proceed exactly the same as if you had requested dim=2.

Yes, meshing up to dim=3 also meshes up to dim=2. I don't have any specific concerns about the codebase, but there could possibly be bugs for example in the way that physical groups are used. I don't think anything in this PR needs to be changed, but integration tests would be great to see in the future.

I would clarify/assert that mesh refinement will discard all volume mesh data

In fact, we're actually discarding volume mesh data as soon as MOABModel.make_from_mesh is called. I added a warning in there to at least note in the log that volume data was discarded.

Sound good!

You're welcome to merge the PR you like

@paulromano paulromano merged commit 5cb2a23 into stellarmesh:main Dec 23, 2023
2 checks passed
@paulromano paulromano deleted the gmsh-3d branch December 23, 2023 06:14
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.

2 participants