Skip to content

Conversation

@mhvk
Copy link

@mhvk mhvk commented Oct 30, 2023

See discussion in numpy#24878

Copy link
Owner

@mdhaber mdhaber left a comment

Choose a reason for hiding this comment

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

@mhvk did you intend to update the meson submodule here?

Also, can you explain why assert_, which is listed as not recommended in the documentation, is used here? We used to use assert_ in SciPy, but my understanding was that Python assert is preferred now.

Otherwise, the code looks like it avoids turning Python scalars into NumPy types, and it makes some other simplfications. Typically when I set out to take an inactive PR over the finish line, I try to minimize changes to keep the review burden low - but it's great that you've used this as an opportunity to improve the function.

I'd suggest that someone more familiar with the intricacies review the tests when this makes its way into the NumPy PR.

@mhvk
Copy link
Author

mhvk commented Nov 8, 2023

@mdhaber - sorry, I must have committed something that was in a branch unintentionally. I made this in rather hurry so feel free to amend the commit (e.g., I'm fairly certain I just copied the assert_ from a test above or below).

mdhaber pushed a commit that referenced this pull request Apr 24, 2024
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