-
Notifications
You must be signed in to change notification settings - Fork 108
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
Implement insert points #69
base: main
Are you sure you want to change the base?
Conversation
…r/meshpy into implement-insert-points
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 working on this! Just one small request, then this should be ready to go.
examples/test_insert_points.py
Outdated
from meshpy.tet import MeshInfo, Options, build | ||
|
||
if __name__ == '__main__': | ||
logger = logging.getLogger('test_insert_points.py') |
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.
Could you add a version of this to the (pytest-driven) tests under test/
, to make sure this doesn't get inadvertently broken?
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.
Done?
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.
Looks good. Please also take care of the Flake8 linter failures.
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.
Got it. Fixed.
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.
Bump
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.
Didn't you mention in #70 that this PR was no longer needed? Could you explain?
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.
Yes. But this appears to be the correct way to implement this and IMO it makes sense to cover tetgen's interface regardless of whether there are parts of it that are a little redundant. Some people looking for a wrapper library for tetgen may be familiar with one approach and not the other.
I made a few small changes to implement TetGen's "constrained points" feature (addressing #60 and #68).
I'm not up on the internals of TetGen, so can't be completely confident that this works, but it seems to work. I added a simple example testing the feature under
examples
.Running
test_insert_points.py
under IPython, I get: