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 ArrayView and numpy subclassing #335

Merged
merged 6 commits into from
Mar 20, 2020
Merged

Fix ArrayView and numpy subclassing #335

merged 6 commits into from
Mar 20, 2020

Conversation

michalk8
Copy link
Contributor

@michalk8 michalk8 commented Mar 6, 2020

Hi,
I have a numpy.ndarray subclass with some column annotations (also ndarrays), including a
view-like object. Problem is that taking view of adata object first converts this to np.ndarray,
effectively removing my subclass (arr = np.asarray(input_array)). This PR fixes this such that only non-arrays are converted to arrays, rest is left the same.

Copy link
Member

@ivirshup ivirshup left a comment

Choose a reason for hiding this comment

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

Thanks for the PR.

I've made a suggestion about a possible shorter change.

Is this something you could easily test for? If you don't want it to accidentally get broken in the future, adding a test would be important.

Comment on lines 72 to 74
if not isinstance(input_array, np.ndarray):
input_array = np.array(input_array)
arr = input_array.view(cls)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if not isinstance(input_array, np.ndarray):
input_array = np.array(input_array)
arr = input_array.view(cls)
arr = np.asanyarray(input_array).view(cls)

Could this work instead? I think it should if your class is literally a np.ndarray subclass.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion! I've added a test and it works.

@michalk8
Copy link
Contributor Author

Any news on this? I wanted to fix the code-quality check (just for the test - either get overriding builtin when using type as an argument or parameters differ...).
@ivirshup would be great if you think everything is on point and then merge this.

@ivirshup
Copy link
Member

Sorry, just been busy! I think this looks good, thanks!

@ivirshup ivirshup merged commit 7fc5c5e into scverse:master Mar 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants