Skip to content

Adapt to PyTorch explicitly representing optionality#2698

Closed
smessmer wants to merge 1 commit intopytorch:masterfrom
smessmer:49138
Closed

Adapt to PyTorch explicitly representing optionality#2698
smessmer wants to merge 1 commit intopytorch:masterfrom
smessmer:49138

Conversation

@smessmer
Copy link
Copy Markdown
Collaborator

PyTorch now explicitly represents optionality of Tensors in operator functions. This changes the signature for the indexing operators since they take lists of optional tensors.

@smessmer smessmer changed the title 49138 Adapt to PyTorch explicitly representing optionality Dec 18, 2020
@smessmer smessmer force-pushed the 49138 branch 6 times, most recently from c672e6b to 683bbdc Compare December 21, 2020 21:27
Copy link
Copy Markdown
Collaborator

@JackCaoG JackCaoG left a comment

Choose a reason for hiding this comment

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

Mostly LGTM

Comment on lines +243 to +248
auto indices_vec = at::expand_outplace(ExpandByteTensors(base, orig_indices));
torch::List<c10::optional<at::Tensor>> indices;
indices.reserve(indices_vec.size());
for (at::Tensor i : std::move(indices_vec)) {
indices.push_back(std::move(i));
}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why couldn't we just pass indices_vec? I didn;t find the function prototype change of expand_outplace in the linked pytorch pr nor can I find the expand_outplace that takes c10::List<c10::optional<at::Tensor>>& in https://github.com/pytorch/pytorch/blob/71ca600af93b4ad58ca6fc083fa5d42a56b43194/aten/src/ATen/ExpandUtils.h. What am I missing here?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

you're right, I changed it

@smessmer smessmer force-pushed the 49138 branch 2 times, most recently from 20ce88a to b586262 Compare December 29, 2020 01:24
@smessmer smessmer requested a review from JackCaoG December 29, 2020 01:24
Copy link
Copy Markdown
Collaborator

@JackCaoG JackCaoG left a comment

Choose a reason for hiding this comment

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

Thanks @smessmer! Feel free to remove the pin and merge it when pytorch pr is merged.

@smessmer
Copy link
Copy Markdown
Collaborator Author

smessmer commented Jan 4, 2021

merged in 3aebd8a

@smessmer smessmer closed this Jan 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants