Skip to content

Conversation

peytondmurray
Copy link
Contributor

@peytondmurray peytondmurray commented Mar 8, 2023

This PR adds support for argmax. I've also modified the signature of the PyArray_CompareFunc to match the documentation. Additionally, compare is now safe against elements with NULL data, e.g. x = np.empty(10); x.sort() is now safe.

Finally, there was an issue with isort in the pre-commit hooks. Autoupdating the hooks resolves this.

@peytondmurray peytondmurray requested a review from ngoldbaum March 8, 2023 19:52
@peytondmurray peytondmurray force-pushed the pyarray-arrfunc-argmax branch from 6812eea to 9b62415 Compare March 8, 2023 19:53
@peytondmurray peytondmurray force-pushed the pyarray-arrfunc-argmax branch 2 times, most recently from 3aefcab to 5c71053 Compare March 9, 2023 00:54
@peytondmurray
Copy link
Contributor Author

peytondmurray commented Mar 9, 2023

I think the tests are failing because the numpy PR on which this PR depends was merged today, and hasn't yet been built in the nightly wheels. Local tests pass for me.

@ngoldbaum
Copy link
Member

Yeah, it is a little annoying we need to wait on that.

We could bring back #4 and build numpy from source. Installing from the wheel was so much faster I dropped it.

@peytondmurray
Copy link
Contributor Author

Ehh, let's just be patient. I hate waiting forever for CI to run, and this only comes up once in a while.

@seberg
Copy link
Member

seberg commented Mar 9, 2023

Triggered a build. But maybe we should adjust the numpy wheels.yml to just run daily, I feel we have enough users (plus you here) that a daily build isn't a waste of resources anymore.

@peytondmurray peytondmurray force-pushed the pyarray-arrfunc-argmax branch from 5c71053 to cb10af0 Compare March 10, 2023 21:28
@peytondmurray
Copy link
Contributor Author

Retriggered the CI, looks good now :)

@ngoldbaum ngoldbaum merged commit 0bd2328 into numpy:main Mar 10, 2023
@peytondmurray peytondmurray deleted the pyarray-arrfunc-argmax branch March 10, 2023 21:55
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.

3 participants