Skip to content

min() and max() support for np.datetime and np.timedelta - #7836

Merged
sklam merged 13 commits into
numba:mainfrom
benwilliamgraham:datetime_min_max
May 26, 2022
Merged

min() and max() support for np.datetime and np.timedelta#7836
sklam merged 13 commits into
numba:mainfrom
benwilliamgraham:datetime_min_max

Conversation

@benwilliamgraham

@benwilliamgraham benwilliamgraham commented Feb 11, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

Fix for #4134

njriasan
njriasan reviewed Feb 14, 2022
View reviewed changes

@njriasan njriasan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This looks pretty good Ben. I left some comments with some changes.

In addition could you change the title of this PR. As it is right now I think the name is unclear, but I think a name like min() and max() support for np.datetime and np.timedelta makes it much easier to understand what this PR contains.

Comment thread numba/cpython/builtins.py Outdated
Comment thread numba/tests/test_npdatetime.py Outdated
Comment thread numba/tests/test_npdatetime.py Outdated
Comment thread numba/tests/test_npdatetime.py
@benwilliamgraham benwilliamgraham changed the title Datetime min max min() and max() support for np.datetime and np.timedelta Feb 14, 2022
@benwilliamgraham
benwilliamgraham force-pushed the datetime_min_max branch 2 times, most recently from 66b8e5d to 75f86bf Compare February 15, 2022 17:36
njriasan
njriasan reviewed Feb 15, 2022
View reviewed changes

@njriasan njriasan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@benwilliamgraham I left some feedback on what I think are possible issues. Good work so far!

Comment thread numba/parfors/parfor.py Outdated
Comment thread numba/parfors/parfor.py Outdated
Comment thread numba/parfors/parfor.py Outdated
Comment thread numba/parfors/parfor.py Outdated
Comment thread numba/parfors/parfor.py Outdated
Comment thread numba/parfors/parfor.py Outdated
Comment thread numba/tests/test_npdatetime.py
njriasan
njriasan reviewed Feb 18, 2022
View reviewed changes

@njriasan njriasan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Overall I think this looks pretty good. Have you determined the source of the CI failure?

Comment thread numba/parfors/parfor.py Outdated
njriasan
njriasan reviewed Feb 18, 2022
View reviewed changes

@njriasan njriasan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I left a couple of comments but it looks very close. Thank you Ben!

Comment thread numba/core/typing/npdatetime.py Outdated
Comment thread numba/np/npdatetime.py Outdated
benwilliamgraham and others added 3 commits February 18, 2022 16:41
@benwilliamgraham @njriasan
Update numba/core/typing/npdatetime.py
b7d3048 
Co-authored-by: Nick Riasanovsky <njriasanovsky@berkeley.edu>
@benwilliamgraham @njriasan
Update numba/np/npdatetime.py
dd32396 
Co-authored-by: Nick Riasanovsky <njriasanovsky@berkeley.edu>
@benwilliamgraham
Fixed max
07276e0
@benwilliamgraham
benwilliamgraham marked this pull request as ready for review February 18, 2022 22:13
njriasan
njriasan previously approved these changes Feb 18, 2022
View reviewed changes

@njriasan njriasan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGMT @benwilliamgraham. Thank you!

@benwilliamgraham

benwilliamgraham commented Feb 22, 2022

Copy link
Copy Markdown
Contributor Author

@stuartarchibald could you have the label switched from "in progress" to "ready for review" please?

@stuartarchibald stuartarchibald added the Effort - medium Medium size effort needed label Feb 22, 2022
sklam
sklam reviewed Feb 23, 2022
View reviewed changes
Comment thread numba/tests/test_npdatetime.py
Comment thread numba/tests/test_npdatetime.py
Comment thread numba/core/typing/builtins.py Outdated
@sklam sklam added 4 - Waiting on author Waiting for author to respond to review and removed 3 - Ready for Review labels Feb 23, 2022
@esc

esc commented Mar 3, 2022

Copy link
Copy Markdown
Member

@benwilliamgraham thank you for working on this, your efforts to improve Numba are appreciated. A quick note about force-pushes. In general, we discourage contributors to force push after a review has begun. This is because our experience has shown, that this makes it harder for Github to line up the comments and also makes it harder for reviewers to follow what has been changed etc.. Thank you in advance for complying with this practice! 🙏

@stuartarchibald stuartarchibald added 4 - Waiting on reviewer Waiting for reviewer to respond to author and removed 4 - Waiting on author Waiting for author to respond to review labels Mar 15, 2022
@njriasan

njriasan commented May 13, 2022

Copy link
Copy Markdown
Contributor

@esc Is it possible to get this added to the 0.56 milestone?

@esc

esc commented May 13, 2022

Copy link
Copy Markdown
Member

@njriasan it looks like it's pretty far along and probably only needs a sign-off from @sklam ? I'll add it it proactively for now (we can always remove it again, in case things don't line up).

@esc esc added this to the Numba 0.56 RC milestone May 13, 2022
sklam
sklam reviewed May 17, 2022
View reviewed changes
Comment thread numba/core/typing/npdatetime.py Outdated
Comment thread numba/core/typing/builtins.py Outdated
@sklam sklam added 4 - Waiting on author Waiting for author to respond to review and removed 4 - Waiting on reviewer Waiting for reviewer to respond to author labels May 17, 2022
@benwilliamgraham
benwilliamgraham requested a review from sklam May 19, 2022 17:48
sklam
sklam reviewed May 24, 2022
View reviewed changes

@sklam sklam left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

One last thing

Comment thread numba/core/typing/builtins.py Outdated
@benwilliamgraham @sklam
Update numba/core/typing/builtins.py
4017346 
Co-authored-by: Siu Kwan Lam <1929845+sklam@users.noreply.github.com>
@benwilliamgraham
benwilliamgraham requested a review from sklam May 24, 2022 16:12
sklam
sklam approved these changes May 25, 2022
View reviewed changes

@sklam sklam left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the patch!

@sklam sklam added 5 - Ready to merge Review and testing done, is ready to merge and removed 4 - Waiting on author Waiting for author to respond to review labels May 25, 2022
@sklam
sklam merged commit ef828dd into numba:main May 26, 2022
@stuartarchibald stuartarchibald mentioned this pull request Jan 5, 2023
Closed
2 tasks done
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sklam sklam sklam approved these changes
@DrTodd13 DrTodd13 Awaiting requested review from DrTodd13 DrTodd13 is a code owner
@stuartarchibald stuartarchibald Awaiting requested review from stuartarchibald
@esc esc Awaiting requested review from esc
+1 more reviewer
@njriasan njriasan njriasan left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

5 - Ready to merge Review and testing done, is ready to merge Effort - medium Medium size effort needed

Projects

None yet

Milestone

Numba 0.56 RC

Development

Successfully merging this pull request may close these issues.

5 participants

@benwilliamgraham @esc @njriasan @sklam @stuartarchibald