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

Add find_cells_intersecting_line #4399

Merged
merged 11 commits into from
May 12, 2023
Merged

Conversation

MatthewFlamm
Copy link
Contributor

@MatthewFlamm MatthewFlamm commented May 9, 2023

Overview

Closes #4398. This PR first fixes the description of find_cells_along_line to clarify that it is finding cells whose bounds intersect the line and not the cells themselves. The PR adds find_cells_intersecting_line to find cells that directly intersect the lines.

Details

I updated the examples to be better. The original example put the line in the (0, 0, 1) direction which should go through the vertex at the top of the sphere where many many cells converge together. It is better to use the (1, 0, 0) direction where it should only intersect 1 cell. The examples now highlight the difference between the methods where find_cells_along_line returns multiple cells, whereas find_cells_intersecting_line returns only 1 cell.

@github-actions github-actions bot added the enhancement Changes that enhance the library label May 9, 2023
@MatthewFlamm
Copy link
Contributor Author

cc @AsimAl-Sofi if you want to test this PR

@codecov
Copy link

codecov bot commented May 9, 2023

Codecov Report

Merging #4399 (004c3c7) into main (3e9be97) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #4399   +/-   ##
=======================================
  Coverage   95.82%   95.82%           
=======================================
  Files          97       97           
  Lines       20874    20890   +16     
=======================================
+ Hits        20002    20018   +16     
  Misses        872      872           

raise VTKVersionError("pyvista.PointSet requires VTK >= 9.2.0")

if np.array(pointa).size != 3:
raise TypeError("Point A must be a length three tuple of floats.")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I followed the other method here, but I think it is probably better as ValueError. If so, we should change both which would be a small breaking change.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree that ValueError would be correct here. It's out of scope of this PR, but let's do it in a different PR and have it merged for 0.40.0

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well with the current wording ("length three tuple of floats") the type error is also correct. Which is just to say that we should probably reword it while we're at it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might consider reusing:

def check_valid_vector(point, name=''):
"""Check if a vector contains three components."""
if not isinstance(point, Iterable):
raise TypeError(f'{name} must be a length three iterable of floats.')
if len(point) != 3:
if name == '':
name = 'Vector'
raise ValueError(f'{name} must be a length three iterable of floats.')

Copy link
Member

@akaszynski akaszynski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Solid LGTM. Thanks @MatthewFlamm.

@akaszynski akaszynski merged commit 244fb80 into main May 12, 2023
24 checks passed
@akaszynski akaszynski deleted the feat/intersect-cells-along-line branch May 12, 2023 03:12
@banesullivan banesullivan mentioned this pull request Jun 30, 2023
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Changes that enhance the library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

find_cells_along_line not working according to description
3 participants