BUG: support axis sequence in np.trim_zeros
#29947
Conversation
34d1558 to
2a604d8
Compare
np.trim_zeros: add support for axis sequence
np.trim_zeros: add support for axis sequencenp.trim_zeros
There was a problem hiding this comment.
Thanks! Sorry for taking a little while to look at this. These sorts of changes to the Python parts of NumPy are always a little inscrutable.
It's borderline but it would probably help any possibly confused users to add a release note. particularly for the axis=None change if that was intentional.
EDIT: removed an incorrect statement
|
Thanks! I'm happy to expand test coverage if you'd like that here – let me know! |
|
Go ahead if you'd like. It would make me feel a little more confident hitting the merge button without someone else looking at it. A release note about the fix/new feature would be nice too. |
2a604d8 to
93aa411
Compare
|
The last commit adds some more comprehensive tests. I added a utility to construct (input, output) pairs in order to avoid having to manually define each test case; let me know if you think this approach is too complicated. |
93aa411 to
ccadee7
Compare
ngoldbaum
left a comment
There was a problem hiding this comment.
The new test is complicated but easy enough to understand with the comments or by stepping through in a debugger. I'm fine with it. Just one comment about the type hints. It'd also still be really nice to get a release note for this.
| def test_multiple_axes(self, shape, axis, trim): | ||
| rng = np.random.default_rng(4321) | ||
| data, expected = self.construct_input_output(rng, shape, axis, trim) | ||
| assert_array_equal(trim_zeros(data, axis=axis, trim=trim), expected) |
There was a problem hiding this comment.
this new test takes 0.35 s to run on my M3 macbook pro. I think that's acceptable.
Done. |
|
Thanks @jakevdp! |
Fixes #29945