Skip to content

Fix split_size test failures#11051

Closed
driazati wants to merge 2 commits intopytorch:masterfrom
driazati:split_size
Closed

Fix split_size test failures#11051
driazati wants to merge 2 commits intopytorch:masterfrom
driazati:split_size

Conversation

@driazati
Copy link
Contributor

@driazati driazati commented Aug 30, 2018

This PR fixes #8525 by renaming split_with_sizes to split so that 2 aten::split ops are
generated (previously aten::split(self, int, int) and aten::split_with_sizes(self, int[], int) were generated)

split_with_sizes was made in PR #5443, but I don't see a reason for it to have
a different name than split rather than just overload split.

This PR fixes #8525 by adding register_special_ops.cpp to mirror Python dispatching from split to split and split_with_sizes in tensor.py.

It also fixes #8520 by adding an int[] wherever it sees torch.Size

In a follow up PR this could also be used to fix some of the other unknown builtin op test errors.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

driazati has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@driazati
Copy link
Contributor Author

cc @ssnl @zdevito

@ssnl
Copy link
Collaborator

ssnl commented Aug 31, 2018

@driazati We separated split and split_with_sizes because split_with_sizes behaves different with np.split (when passed with a list as an argument). So making it also called split will be confusing.

@driazati
Copy link
Contributor Author

driazati commented Sep 5, 2018

@pytorchbot retest this please

Summary:

Test Plan:

Reviewers:

Subscribers:

Tasks:

Tags:
@driazati
Copy link
Contributor Author

driazati commented Sep 6, 2018

re-cc @ssnl @apaszke @zdevito

namespace {
RegisterOperators reg({
Operator(
"aten::split(Tensor self, int[] split_sizes, int dim=0) -> Tensor[]",

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

Operator(
"aten::Size(int[] split_sizes) -> int[]",
[](Stack& stack) {
auto result = (std::move(peek(stack, 0, 1))).toIntList()->elements();

This comment was marked as off-topic.

namespace {
RegisterOperators reg({
Operator(
"aten::split(Tensor self, int[] split_sizes, int dim=0) -> Tensor[]",

This comment was marked as off-topic.

Summary:

Test Plan:

Reviewers:

Subscribers:

Tasks:

Tags:
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

driazati has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

return 0;
}),
Operator(
"aten::Size(int[] sizes) -> int[]",

This comment was marked as off-topic.

This comment was marked as off-topic.

PenghuiCheng pushed a commit to PenghuiCheng/pytorch that referenced this pull request Sep 11, 2018
Summary:
~~This PR fixes pytorch#8525 by renaming `split_with_sizes` to `split` so that 2 `aten::split` ops are
generated (previously `aten::split(self, int, int)` and `aten::split_with_sizes(self, int[], int)` were generated)~~

~~`split_with_sizes` was made in PR pytorch#5443, but I don't see a reason for it to have
a different name than `split` rather than just overload `split`.~~

This PR fixes pytorch#8525 by adding `register_special_ops.cpp` to mirror Python dispatching from `split` to `split` and `split_with_sizes` in [tensor.py](https://github.com/pytorch/pytorch/blob/master/torch/tensor.py#L279).

It also fixes pytorch#8520 by adding an `int[]` wherever it sees `torch.Size`

In a follow up PR this could also be used to fix some of the other `unknown builtin op` test errors.
Pull Request resolved: pytorch#11051

Differential Revision: D9582443

Pulled By: driazati

fbshipit-source-id: d27201f85937d72e45e851eaa1460dd3dd1b61a9
@ezyang ezyang added the merged label Jun 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

6 participants