-
Notifications
You must be signed in to change notification settings - Fork 443
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
Ensure no overlap with crinkle clip #6060
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6060 +/- ##
=======================================
Coverage 96.96% 96.96%
=======================================
Files 141 141
Lines 24530 24532 +2
=======================================
+ Hits 23786 23788 +2
Misses 744 744 |
I am +1 for including this feature, and it makes sense based on the description of the parameters. But, are there also any use cases for returning the crinkle cut from both directions here (current behavior)? I can't think of any obvious ones, but it seems probable that someone would want this behavior. |
I also cannot think of any and I think that when calling the clip function with crinkle enabled and wanting both sides of the clip, there should not be any overlap. If someone wants to crinkle both sides on the overlap, then thats still possible, its just two seperate clips and can be done two ways:
a = dataset.clip('y', crinkle=True)
b = dataset.clip('-y', crinkle=True)
a = dataset.clip('y', crinkle=True, invert=True)
b = dataset.clip('y', crinkle=True, invert=False) import pyvista as pv
from pyvista import examples
dataset = examples.download_bunny_coarse()
clipped, unclipped = dataset.clip('y', return_clipped=True, crinkle=True)
a = dataset.clip('y', crinkle=True)
b = dataset.clip('-y', crinkle=True)
# a = dataset.clip('y', crinkle=True, invert=True)
# b = dataset.clip('y', crinkle=True, invert=False)
assert (a == clipped)
assert not (b == unclipped)
pl = pv.Plotter(shape=(1,2))
pl.add_mesh(clipped, color='blue')
pl.add_mesh(unclipped, color='red', style='wireframe', line_width=5)
pl.subplot(0, 1)
pl.add_mesh(a, color='blue')
pl.add_mesh(b, color='red', style='wireframe', line_width=5)
pl.link_views()
pl.show() |
I think it is fine as the PR has it. There could be an unexpected breaking change for someone using the current behavior. We might argue it was unintended behavior, so they should not have been using it? I'm mostly trying to think through whether a 'breaking change' label should be applied, or this is just a bug fix. Not trying to change the PR itself. I thought of one case, two crinkle cuts that are offset in space are used in opposing directions to obtain a sliver of cells to compare cell size distribution (common in CFD for example). Currently, a user could achieve this by either inverting one clip or using |
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.
Small suggestion to consider. Also consider labeling breaking change. Im only 50/50 on both.
While in my opinion this is a bug and unintended behavior I think you provide a great example and I want to note that this bug has been around for 2 years (ref #2316). I think a breaking change label to denote that the output mesh is indeed different is appropriate here. |
* Ensure no overlap with crinkle clip * Improve test
Overview
Resolves #5667 to ensure that when crinkle clipping and returning the clipped out mesh, the clipped out mesh does not have any of the same cells as the primary.
Details