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

np.trim_zeros #5543

Closed
wants to merge 12 commits into from
Closed

np.trim_zeros #5543

wants to merge 12 commits into from

Conversation

sungraek
Copy link
Contributor

Add support for np.trim_zeros. It currently does not handle nested list or tuple input well.
#4074

@gmarkall
Copy link
Member

Thanks very much for the PR, I've queued it for review.

@sklam sklam changed the title [WIP] np.trim_zeros np.trim_zeros Apr 13, 2020
@stuartarchibald stuartarchibald added this to the Numba 0.50 RC milestone May 14, 2020
@gmarkall gmarkall self-requested a review May 15, 2020 22:02
def impl(a, trim='fb'):
a_ = np.asarray(a)
first = 0
if 'f' in trim:
Copy link
Contributor

Choose a reason for hiding this comment

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

From this line it seems the numpy implementation works with upper and lower case letters for trim

>>> np.trim_zeros(np.array([0, 1, 2, 3, 0, 0]), trim="FB")
array([1, 2, 3])

yield np.zeros(1)
yield np.array([1, 2, 3])
yield np.array([0, 1, 2, 3])
yield np.array([0, 1, 2, 0, 0])
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we test arrays that have non-zero values that are not ints (e.g. np.array([0., 1., 2., np.nan, 0.]))?
We could also test other data types that work with the numpy version (e.g. np.array(["Hello", "world"])).

yield np.array([0, 1, 2, 0, 0])

pyfunc = np_trim_zeros
cfunc = jit(nopython=True)(pyfunc)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we also test when trim != "fb"?

@EPronovost
Copy link
Contributor

Nice work! You'll probably want to update the numpy supported features documentation as part of your PR as well.

@sungraek
Copy link
Contributor Author

I added support for both lower and upper case arguments and some new test cases as you mentioned.

@gmarkall gmarkall removed their request for review May 18, 2020 15:54
@stuartarchibald stuartarchibald added 4 - Waiting on author Waiting for author to respond to review and removed 3 - Ready for Review labels Aug 26, 2020
@ionikha
Copy link

ionikha commented Feb 24, 2023

Hi, i'm working on a project that uses trim_zeros and numba, do you know why this wasn't added?
It looks good to go?

@gmarkall
Copy link
Member

gmarkall commented Mar 6, 2023

@ionikha Thanks for the ping, I think it just fell by the wayside and never got a review from a maintainer (even though @EPronovost very kindly gave it a review). I've added this to the agenda for the dev meeting tomorrow to see if we can kickstart it.

@gmarkall
Copy link
Member

gmarkall commented Mar 7, 2023

Assignment from triage meeting: @gmarkall to merge main and address any review comments, @guilhermeleobas to review subsequently.

@gmarkall gmarkall added 4 - Waiting on reviewer Waiting for reviewer to respond to author 4 - Waiting on second reviewer Patch needs a second reviewer. and removed 4 - Waiting on author Waiting for author to respond to review labels Mar 7, 2023
@guilhermeleobas
Copy link
Collaborator

@gmarkall, if needed, we can switch and I can merge main and address any comments. Just let me know

@esc
Copy link
Member

esc commented Jun 27, 2023

@gmarkall @guilhermeleobas -- can I ask for an update here? What is the status of this?

@gmarkall
Copy link
Member

I'll put this on my near-term todo list so that it can get done.

@guilhermeleobas
Copy link
Collaborator

@gmarkall, I've created #9074 which is built on top of this PR with git conflicts fixed.

@esc
Copy link
Member

esc commented Jul 18, 2023

Is replaced by #9074

@esc esc closed this Jul 18, 2023
@esc esc added abandoned PR is abandoned (no reason required) and removed 4 - Waiting on reviewer Waiting for reviewer to respond to author 4 - Waiting on second reviewer Patch needs a second reviewer. labels Jul 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
abandoned PR is abandoned (no reason required)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants