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

Fix get_array return type #2782

Merged
merged 1 commit into from
Jun 15, 2022
Merged

Fix get_array return type #2782

merged 1 commit into from
Jun 15, 2022

Conversation

akaszynski
Copy link
Member

Problem

The documentation for DataSetAttributes.get_array, states that:

Returns
-------
pyvista.pyvista_ndarray or vtkDataArray
    Returns a :class:`pyvista.pyvista_ndarray` if the
    underlying array is either a ``vtk.vtkDataArray`` or
    ``vtk.vtkStringArray``.  Otherwise, returns a
    ``vtk.vtkAbstractArray``.

However, in neither our testing or documentation build do we ever return a vtk.vtkAbstractArray. Moreover, since a vtk.vtkAbstractArray can't be directly instanciated, it's not possible to set this array either. It's also unclear why this was added in the first place.

Solution

Simply remove this branch statement and clean up the return type and docstring.

It's not hit in any of our tests, and this simplifies our API such that we always return a pyvista.pyvista_ndarray from DataSetAttributes.get_array. In the unlikely case where an edge case arises, we can add support for that in a follow-up PR.

@github-actions github-actions bot added the bug Uh-oh! Something isn't working as expected. label Jun 13, 2022
@adeak adeak linked an issue Jun 13, 2022 that may be closed by this pull request
Copy link
Member

@adeak adeak left a comment

Choose a reason for hiding this comment

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

LGTM (with the caveat that we still don't know why this was here).

@codecov
Copy link

codecov bot commented Jun 13, 2022

Codecov Report

Merging #2782 (632fd1e) into main (d5c89f1) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #2782      +/-   ##
==========================================
+ Coverage   93.86%   93.88%   +0.01%     
==========================================
  Files          76       76              
  Lines       16230    16255      +25     
==========================================
+ Hits        15235    15261      +26     
+ Misses        995      994       -1     

@akaszynski
Copy link
Member Author

LGTM (with the caveat that we still don't know why this was here).

Since we should always be returning a "wrapped" array, I'd prefer just handing a vtkAbstractArray pyvista.pyvista_ndarray or raising a TypeError (or potentially a custom not supported error). However, I can't even create an example to return a vtkAbstractArray and therefore can't even come up with a fictitious edge case.

Again @JevinJ, if you could weigh in here as to why we could either preserve or handle this branch better.

@adeak
Copy link
Member

adeak commented Jun 13, 2022

Good point:

>>> pv._vtk.vtkAbstractArray()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: this is an abstract class and cannot be instantiated

So it's very much abstract indeed, and there's no chance to find an object the type of which is exactly vtkAbstractArray. I think this is good to go.

@JevinJ
Copy link
Member

JevinJ commented Jun 13, 2022

So long ago, but I believe this was for an edge case on the vtk side or the user doing something weird. If that's not normal then I guess that makes sense to remove.

@akaszynski akaszynski merged commit dd53797 into main Jun 15, 2022
@akaszynski akaszynski deleted the fix/get_array_return_type branch June 15, 2022 09:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Uh-oh! Something isn't working as expected.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Investigate abstract array type check in DataSetAttributes.get_array()
3 participants