Open
Conversation
cgoates
requested changes
Aug 11, 2025
Owner
cgoates
left a comment
There was a problem hiding this comment.
Hey, @colbyj427, thank you, this looks great! A few thoughts here:
- Sadly, in the end, we don't actually use the connections between the quad mesh and the tet mesh, because we already have some tooling available for this on the C++ side, and it simplified the API to do it on the C++ side. I'm thinking if we remove the python code that does that, we will no longer need the half-edge data structure in python, and it might vastly reduce the amount of code we're adding here. Once we've got that simplified down, let's talk about where to fit these files in the directory structure. In general we want to keep as little code as we can while still retaining the desired functionality. It's okay to keep the deleted code in an earlier commit so we can come back and get it if needed later.
- We want to avoid committing output files. The only obj/vtu/3dm/txt/mtl files we should commit to the repository should be those needed to run tests or examples, and those used for tests should go into the test/data/ folder.
- Will you please add a test in the test/ folder that does something similar to the test_sweep_param.py new function that you added, but without all the old stuff I had in that file? You can see the general pattern for adding a python test to the list that runs using the
ctestcommand intest/CMakeLists.txtat the bottom. Please check that the test fails if the code fails.
cgoates
reviewed
Aug 14, 2025
added 27 commits
August 14, 2025 14:33
…ctionality to remove obj analysis.
…uadMesh class, deleted class.
Owner
|
@colbyj427 This all looks great. The only question I have is, where is the ScaleUntrim repo being installed on the runners for the python test you added? I don't see you running the buildScaleUntrim function anywhere, and so I'm wondering how it can be actually running the quad meshing in your test. It seems like perhaps ScaleUntrim not existing isn't causing the test to fail, unless I'm missing something. Other than that, I think this is ready to go. |
added 26 commits
August 15, 2025 14:25
… to account for moving the file to the scripts folder.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.