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

Make nonzero return Tensors #55

Merged
merged 1 commit into from
May 17, 2024
Merged

Make nonzero return Tensors #55

merged 1 commit into from
May 17, 2024

Conversation

mtsokol
Copy link
Collaborator

@mtsokol mtsokol commented May 17, 2024

Hi @hameerabbasi,

This is a small tweak to make sure nonzero returns Tensor's instead of NumPy arrays.

@mtsokol mtsokol added the enhancement New feature or request label May 17, 2024
@mtsokol mtsokol self-assigned this May 17, 2024
@mtsokol mtsokol merged commit d676a15 into main May 17, 2024
5 checks passed
@mtsokol mtsokol deleted the tweak-nonzero branch May 17, 2024 13:24
@willow-ahrens
Copy link
Collaborator

Is this the desired approach? I'm fine with this but I wonder if users would want a wrapped array when they call these destructuring operators. Usually they call this when the arrays are moving to a different library or framework.

@willow-ahrens
Copy link
Collaborator

Hameer, you've been right before when it comes to conventions like this, so I'll defer to you here. This struck me as strange though since most of the time people call this when they "just want the data"

@hameerabbasi
Copy link
Collaborator

Hameer, you've been right before when it comes to conventions like this, so I'll defer to you here. This struck me as strange though since most of the time people call this when they "just want the data"

Well, what I'd do is compute arr == zero_scalar as a Tensor with a dtype of bool, materialize it as a canonicalized COO, and pull out the coords.

@mtsokol
Copy link
Collaborator Author

mtsokol commented May 17, 2024

I wrapped it because array-api-tests expects results of functions of the same type as inputs. So, Tensor in, Tensor out. When Tensor is an input and NumPy is an output it flags it as an error.

@hameerabbasi
Copy link
Collaborator

I wrapped it because array-api-tests expects results of functions of the same type as inputs. So, Tensor in, Tensor out. When Tensor is an input and NumPy is an output it flags it as an error.

Yes, we should definitely have "this library type" in/out.

@mtsokol
Copy link
Collaborator Author

mtsokol commented May 17, 2024

Hameer, you've been right before when it comes to conventions like this, so I'll defer to you here. This struck me as strange though since most of the time people call this when they "just want the data"

Well, what I'd do is compute arr == zero_scalar as a Tensor with a dtype of bool, materialize it as a canonicalized COO, and pull out the coords.

We already have nonzero implementation on the Finch side, which we're calling here.

@willow-ahrens
Copy link
Collaborator

willow-ahrens commented May 17, 2024

Ah, I see. No worries, we can leave as is if the Array API requires it. Just thought I'd register that this is usually not what the user would expect from a function like this. A related function is "find". In Scipy, "find" does not return a tuple of scipy.sparse arrays, it just returns ndarray. https://docs.scipy.org/doc/scipy/reference/generated/scipy.sparse.find.html#scipy.sparse.find

@willow-ahrens
Copy link
Collaborator

I think most people will find themselves unwrapping the results of this function

@willow-ahrens
Copy link
Collaborator

Maybe if find is not in the API standard, we could introduce a function called find that just returns ndarrays.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants