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

BUG: add int->int loop for trunc #19505

Closed
wants to merge 8 commits into from

Conversation

kshitij12345
Copy link

@kshitij12345 kshitij12345 commented Jul 17, 2021

Fixes: #19464

Not entirely sure if this change is automatically picked up by test_ufunc.py. Would be great to know if this needs more explicit test.

Thanks!

Note: BC-Breaking

cc: @seberg

@kshitij12345 kshitij12345 marked this pull request as ready for review July 17, 2021 10:45
@seberg
Copy link
Member

seberg commented Jul 17, 2021

Thanks, I think this should have a brief release note at least (see doc/release/upcoming_changes there is a README there. And since I am not sure how conservative we should be here: could you ping the mailing list about the change? We could give this a proper FutureWarning, although it would be a bit annoying...

Also needs at least some basic tests (although it looks like the code actually gets executed somewhere already).

@charris charris changed the title [fix] trunc: add int->int loop BUG: add int->int loop for trunc Jul 17, 2021
@kshitij12345
Copy link
Author

could you ping the mailing list about the change?

Sure. Will do that.

Thanks!!

@kshitij12345
Copy link
Author

@seberg
Copy link
Member

seberg commented Jul 19, 2021

This is confusing me, there seem to be some emails there that should be there and were posted to the actual mailing list? But then there are also definitely mails missing and some additional mails (like yours).

But does nabble just keep its own stuff around and it is not connected to the actual mailing list, which is hosted on python.org?

@mattip
Copy link
Member

mattip commented Jul 23, 2021

@kshitij12345 I am not sure how you sent your mail, but I think it did not actually go out to the mailing list even though it somehow shows up in nabble. Could you (perhaps subscribe to https://mail.python.org/mailman/listinfo/numpy-discussion and) re-send the mail to numpy-discussion@python.org ?

@kshitij12345
Copy link
Author

Ah. Sure. Sorry for the confusion.

Thanks @seberg @mattip

@mattip
Copy link
Member

mattip commented Jul 23, 2021

In any case this looks good. The other ufuncs in that family: rint, fix, ceil, floor have the same problem.

@mattip
Copy link
Member

mattip commented Aug 30, 2021

@kshitij12345 did you get a chance to look at the other ufuncs and/or ping the mailing list about this?

@kshitij12345
Copy link
Author

@mattip Ah sorry. I missed doing that. Will do both of them in a day or two. Thanks for the ping :)

@seberg seberg added the 62 - Python API Changes or additions to the Python API. Mailing list should usually be notified. label Sep 10, 2021
@@ -612,6 +612,13 @@ def test_true_divide(self):
assert_(res == 0.0)
assert_(res.dtype.name == 'float64')

def test_trunc_expected_dtypes(self):
# Supported dtypes by trunc.
for tc in 'bBhHiIlLqQefdgO':
Copy link
Member

Choose a reason for hiding this comment

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

What about boolean ?? (What should that actually do?)

Other than that, the only question is whether we should not aim for fixing this for the whole class of truncating functions?

Copy link
Author

Choose a reason for hiding this comment

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

Ah. Right now, bool get promoted which we don't want. Will add a loop entry for bool.

As for others, is it ok to do a follow-up PR.

Apologies for the delayed response. (Will be more active henceforth :))

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
00 - Bug 62 - Python API Changes or additions to the Python API. Mailing list should usually be notified.
Projects
Status: Pending author's response
Development

Successfully merging this pull request may close these issues.

np.trunc is inconsistent with array-api
3 participants