De-prioritise Dimname and DimnameList in python overload resolution#51350
Closed
peterbell10 wants to merge 10 commits intogh/peterbell10/48/basefrom
Closed
De-prioritise Dimname and DimnameList in python overload resolution#51350peterbell10 wants to merge 10 commits intogh/peterbell10/48/basefrom
peterbell10 wants to merge 10 commits intogh/peterbell10/48/basefrom
Conversation
`None` being a valid `Dimname` is awkward for optional `dim` arguments, as found on NumPy's reduction functions like `std` and `var`. In this cases `dim=None` should mean an all-reduction, but instead you get an error "Please look up dimensions by name". I've also had to fix `FunctionParameter::check` to actually check the first element of `INT_LIST` arguments and reject non-int types. Otherwise, the dim names end up calling the `int[]` overload and fail. [ghstack-poisoned]
This was referenced Jan 29, 2021
Contributor
💊 CI failures summary and remediationsAs of commit cc322e2 (more details on the Dr. CI page): 💚 💚 Looks good so far! There are no failures yet. 💚 💚 This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.Please report bugs/suggestions to the (internal) Dr. CI Users group. |
…esolution" `None` being a valid `Dimname` is awkward for optional `dim` arguments, as found on NumPy's reduction functions like `std` and `var`. In these cases `dim=None` should mean an all-reduction, but instead you get an error "Please look up dimensions by name". I've also had to fix `FunctionParameter::check` to actually check the first element of `INT_LIST` arguments and reject non-int types. Otherwise, the dim names end up calling the `int[]` overload and fail. [ghstack-poisoned]
…esolution" `None` being a valid `Dimname` is awkward for optional `dim` arguments, as found on NumPy's reduction functions like `std` and `var`. In these cases `dim=None` should mean an all-reduction, but instead you get an error "Please look up dimensions by name". I've also had to fix `FunctionParameter::check` to actually check the first element of `INT_LIST` arguments and reject non-int types. Otherwise, the dim names end up calling the `int[]` overload and fail. [ghstack-poisoned]
…esolution" `None` being a valid `Dimname` is awkward for optional `dim` arguments, as found on NumPy's reduction functions like `std` and `var`. In these cases `dim=None` should mean an all-reduction, but instead you get an error "Please look up dimensions by name". I've also had to fix `FunctionParameter::check` to actually check the first element of `INT_LIST` arguments and reject non-int types. Otherwise, the dim names end up calling the `int[]` overload and fail. [ghstack-poisoned]
Collaborator
|
@bhosmer I think you're the correct reviewer for this change? |
bhosmer
reviewed
Feb 2, 2021
bhosmer
left a comment
There was a problem hiding this comment.
Given the awkwardness of Dimname scooping up None values, this seems like a reasonable solution for the eager binding layer. I think we'll need to do something similar for the JIT though. cc @eellison for confirmation - Elias, do we support Dimname overloads on the JIT side?
| PySequence_GetItem(obj, 0)); | ||
| // NOTE: JIT tracer allows arbitrary scalar tensors to act as ints | ||
| // in an intlist argument Even float or complex scalars | ||
| return (THPVariable_Check(item.ptr()) || THPUtils_checkIndex(item.ptr())); |
There was a problem hiding this comment.
Be good to factor this out now that it's nontrivial, a la is_float_or_complex_list() just below
bhosmer
reviewed
Feb 2, 2021
bhosmer
left a comment
There was a problem hiding this comment.
Given the awkwardness of Dimname scooping up None values, this seems like a reasonable solution for the eager binding layer. I think we'll need to do something similar for the JIT though. cc @eellison for confirmation - Elias, do we support Dimname overloads on the JIT side?
…esolution" `None` being a valid `Dimname` is awkward for optional `dim` arguments, as found on NumPy's reduction functions like `std` and `var`. In these cases `dim=None` should mean an all-reduction, but instead you get an error "Please look up dimensions by name". I've also had to fix `FunctionParameter::check` to actually check the first element of `INT_LIST` arguments and reject non-int types. Otherwise, the dim names end up calling the `int[]` overload and fail. [ghstack-poisoned]
…esolution" `None` being a valid `Dimname` is awkward for optional `dim` arguments, as found on NumPy's reduction functions like `std` and `var`. In these cases `dim=None` should mean an all-reduction, but instead you get an error "Please look up dimensions by name". I've also had to fix `FunctionParameter::check` to actually check the first element of `INT_LIST` arguments and reject non-int types. Otherwise, the dim names end up calling the `int[]` overload and fail. [ghstack-poisoned]
…esolution" `None` being a valid `Dimname` is awkward for optional `dim` arguments, as found on NumPy's reduction functions like `std` and `var`. In these cases `dim=None` should mean an all-reduction, but instead you get an error "Please look up dimensions by name". I've also had to fix `FunctionParameter::check` to actually check the first element of `INT_LIST` arguments and reject non-int types. Otherwise, the dim names end up calling the `int[]` overload and fail. [ghstack-poisoned]
Contributor
|
@bhosmer i dont think dimname is supported, the schemas with dimname dont get attempted to be matched to in JIT. it probably wouldnt be too hard to add just no one has done it :/ |
Ah got it. Well, I don't think observing this overload ordering will add any difficulty once someone does, at least. Thanks for the info! |
…esolution" `None` being a valid `Dimname` is awkward for optional `dim` arguments, as found on NumPy's reduction functions like `std` and `var`. In these cases `dim=None` should mean an all-reduction, but instead you get an error "Please look up dimensions by name". I've also had to fix `FunctionParameter::check` to actually check the first element of `INT_LIST` arguments and reject non-int types. Otherwise, the dim names end up calling the `int[]` overload and fail. [ghstack-poisoned]
…esolution" `None` being a valid `Dimname` is awkward for optional `dim` arguments, as found on NumPy's reduction functions like `std` and `var`. In these cases `dim=None` should mean an all-reduction, but instead you get an error "Please look up dimensions by name". I've also had to fix `FunctionParameter::check` to actually check the first element of `INT_LIST` arguments and reject non-int types. Otherwise, the dim names end up calling the `int[]` overload and fail. [ghstack-poisoned]
…esolution" `None` being a valid `Dimname` is awkward for optional `dim` arguments, as found on NumPy's reduction functions like `std` and `var`. In these cases `dim=None` should mean an all-reduction, but instead you get an error "Please look up dimensions by name". I've also had to fix `FunctionParameter::check` to actually check the first element of `INT_LIST` arguments and reject non-int types. Otherwise, the dim names end up calling the `int[]` overload and fail. [ghstack-poisoned]
Contributor
aocsa
pushed a commit
to Quansight/pytorch
that referenced
this pull request
Mar 15, 2021
…ytorch#51350) Summary: Pull Request resolved: pytorch#51350 `None` being a valid `Dimname` is awkward for optional `dim` arguments, as found on NumPy's reduction functions like `std` and `var`. In these cases `dim=None` should mean an all-reduction, but instead you get an error "Please look up dimensions by name". I've also had to fix `FunctionParameter::check` to actually check the first element of `INT_LIST` arguments and reject non-int types. Otherwise, the dim names end up calling the `int[]` overload and fail. Test Plan: Imported from OSS Reviewed By: ngimel Differential Revision: D26756208 Pulled By: mruberry fbshipit-source-id: 44221ca0f4822ec2c1f62b092466fd4f779eb45a
xsacha
pushed a commit
to xsacha/pytorch
that referenced
this pull request
Mar 31, 2021
…ytorch#51350) Summary: Pull Request resolved: pytorch#51350 `None` being a valid `Dimname` is awkward for optional `dim` arguments, as found on NumPy's reduction functions like `std` and `var`. In these cases `dim=None` should mean an all-reduction, but instead you get an error "Please look up dimensions by name". I've also had to fix `FunctionParameter::check` to actually check the first element of `INT_LIST` arguments and reject non-int types. Otherwise, the dim names end up calling the `int[]` overload and fail. Test Plan: Imported from OSS Reviewed By: ngimel Differential Revision: D26756208 Pulled By: mruberry fbshipit-source-id: 44221ca0f4822ec2c1f62b092466fd4f779eb45a
laurentdupin
pushed a commit
to laurentdupin/pytorch
that referenced
this pull request
Apr 24, 2026
…ytorch#51350) Summary: Pull Request resolved: pytorch#51350 `None` being a valid `Dimname` is awkward for optional `dim` arguments, as found on NumPy's reduction functions like `std` and `var`. In these cases `dim=None` should mean an all-reduction, but instead you get an error "Please look up dimensions by name". I've also had to fix `FunctionParameter::check` to actually check the first element of `INT_LIST` arguments and reject non-int types. Otherwise, the dim names end up calling the `int[]` overload and fail. Test Plan: Imported from OSS Reviewed By: ngimel Differential Revision: D26756208 Pulled By: mruberry fbshipit-source-id: 44221ca0f4822ec2c1f62b092466fd4f779eb45a
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 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.
Stack from ghstack:
Nonebeing a validDimnameis awkward for optionaldimarguments, as foundon NumPy's reduction functions like
stdandvar. In these casesdim=Noneshould mean an all-reduction, but instead you get an error
"Please look up dimensions by name".
I've also had to fix
FunctionParameter::checkto actually check the firstelement of
INT_LISTarguments and reject non-int types. Otherwise, the dimnames end up calling the
int[]overload and fail.Differential Revision: D26756208