[jit] Made floor/ceil return ints (yay Python 3!) #21111
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
|
We have to be careful not to break BC here |
Contributor
|
Yea agree with @elias Ellison<mailto:eellison@fb.com> . We will need to label this as bc breaking if the original implementation was in 1.1.0 release.
Although in general I feel it’s good to align our behavior with py3 as early as possible. @horace He<mailto:chilli@fb.com> would you mind give it a quick check and label this PR if necessary? Thanks!!
Thanks,
Ailing
…________________________________
From: eellison <notifications@github.com>
Sent: Wednesday, May 29, 2019 6:11:58 PM
To: pytorch/pytorch
Cc: Ailing Zhang; Review requested
Subject: Re: [pytorch/pytorch] [jit] Made floor/ceil return ints (yay Python 3!) (#21111)
We have to be careful not to break BC here
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub<#21111?email_source=notifications&email_token=ABIBI6VMT6FOOZD26XNLQ33PX4SV5A5CNFSM4HQZPCUKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWRBXXY#issuecomment-497163231>, or mute the thread<https://github.com/notifications/unsubscribe-auth/ABIBI6UAAKP73MOYLWDTCN3PX4SV5ANCNFSM4HQZPCUA>.
|
Collaborator
Author
|
I believe the original implementation was checked in before 1.1.0. Does it actually break backwards compatibility though? Ints are implicitly upcasted to floats when necessary right? I suppose there could always be code that called |
Contributor
ints arent upcasted to floats |
Collaborator
Author
|
Closing this PR in favor of the stack I'm about to push. |
Collaborator
Author
|
This is the new PR: #21124 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In particular, I think
flooor/ceilshould have special handling (even if we silently fail for other functions) as they're returning integers. Havingpow(0, -2)returninfis somewhat reasonable, havingfloor(NaN)return -951347564385 (or something like that) is less so.