Skip to content

BUG: support axis sequence in np.trim_zeros#29947

Merged
ngoldbaum merged 4 commits intonumpy:mainfrom
jakevdp:fix-trimzeros
Oct 22, 2025
Merged

BUG: support axis sequence in np.trim_zeros#29947
ngoldbaum merged 4 commits intonumpy:mainfrom
jakevdp:fix-trimzeros

Conversation

@jakevdp
Copy link
Copy Markdown
Contributor

@jakevdp jakevdp commented Oct 14, 2025

Fixes #29945

@jakevdp jakevdp self-assigned this Oct 14, 2025
@jakevdp jakevdp force-pushed the fix-trimzeros branch 2 times, most recently from 34d1558 to 2a604d8 Compare October 14, 2025 11:07
@jakevdp jakevdp removed their assignment Oct 14, 2025
@jakevdp jakevdp changed the title np.trim_zeros: add support for axis sequence np.trim_zeros: add support for axis sequence Oct 16, 2025
@jakevdp jakevdp changed the title np.trim_zeros: add support for axis sequence BUG: support axis sequence in np.trim_zeros Oct 16, 2025
@jakevdp jakevdp mentioned this pull request Oct 20, 2025
Closed
ngoldbaum
ngoldbaum requested changes Oct 20, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@ngoldbaum ngoldbaum left a comment •
edited
Loading

Choose a reason for hiding this comment

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

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

ngoldbaum
ngoldbaum approved these changes Oct 20, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@ngoldbaum ngoldbaum left a comment •
edited
Loading

Choose a reason for hiding this comment

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

NM, this is OK to merge. I confused myself. Added a 2.4.0 milestone to make sure this gets merged before the next release.

@ngoldbaum ngoldbaum added this to the 2.4.0 release milestone Oct 20, 2025
@jakevdp
Copy link
Copy Markdown
Contributor Author

jakevdp commented Oct 20, 2025

Thanks! I'm happy to expand test coverage if you'd like that here – let me know!

@ngoldbaum
Copy link
Copy Markdown
Member

ngoldbaum commented Oct 21, 2025

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.

@jakevdp
Copy link
Copy Markdown
Contributor Author

jakevdp commented Oct 21, 2025 •
edited
Loading

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.

ngoldbaum
ngoldbaum reviewed Oct 22, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@ngoldbaum ngoldbaum left a comment

Choose a reason for hiding this comment

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

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.

numpy/lib/tests/test_function_base.py
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)
Copy link
Copy Markdown
Member

@ngoldbaum ngoldbaum Oct 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

this new test takes 0.35 s to run on my M3 macbook pro. I think that's acceptable.

@jakevdp
Copy link
Copy Markdown
Contributor Author

jakevdp commented Oct 22, 2025

It'd also still be really nice to get a release note for this.

Done.

@ngoldbaum
Copy link
Copy Markdown
Member

ngoldbaum commented Oct 22, 2025

Thanks @jakevdp!

@ngoldbaum ngoldbaum merged commit 76e9118 into numpy:main Oct 22, 2025
76 checks passed
@jakevdp jakevdp deleted the fix-trimzeros branch October 22, 2025 23:01
IndifferentArea pushed a commit to IndifferentArea/numpy that referenced this pull request Dec 7, 2025
@jakevdp @IndifferentArea
BUG: support axis sequence in np.trim_zeros (numpy#29947)
72a922e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ngoldbaum ngoldbaum ngoldbaum approved these changes

Assignees

No one assigned

Labels

00 - Bug

Projects

None yet

Milestone

2.4.0 release

Development

Successfully merging this pull request may close these issues.

BUG: trim_zeros fails with sequence of axes

2 participants

@jakevdp @ngoldbaum