Skip to content

Rename getTypeOpt to getNonVariableTypeOpt#11077

Closed
ezyang wants to merge 1 commit intomasterfrom
export-D9578398
Closed

Rename getTypeOpt to getNonVariableTypeOpt#11077
ezyang wants to merge 1 commit intomasterfrom
export-D9578398

Conversation

@ezyang
Copy link
Contributor

@ezyang ezyang commented Aug 30, 2018

Rename getTypeOpt to getNonVariableTypeOpt

getType now supports retrieving variable types, so make it clearer
when a getType function does NOT give you a variable type.

codemod -d . --extensions cc,cpp,cu,cuh,h getTypeOpt getNonVariableTypeOpt

Differential Revision: D9578398

Differential Revision: D9578398
Differential Version: 56517363
// Precondition: Access to this struct is protected by the GIL
at::Type* aten_type() {
if (!aten_type_) {
auto* baseType = globalContext().getTypeOpt(static_cast<at::Backend>(backend), static_cast<at::ScalarType>(scalar_type));

This comment was marked as off-topic.


at::Type& getType(at::ScalarType scalarType, const THPLayout& layout, const at::Device& device) {
const at::Backend backend = get_backend(device.type() == at::Device::Type::CUDA, layout.layout == at::Layout::Sparse);
auto baseType = at::globalContext().getTypeOpt(backend, scalarType);

This comment was marked as off-topic.

, sparseDims_(1)
, denseDims_(0)
, indices_(globalContext().getTypeOpt(sparseTensorIdToDenseBackend(type_id), ScalarType::Long)->tensor({1, 0}))
, values_(globalContext().getTypeOpt(sparseTensorIdToDenseBackend(type_id), scalar_type)->tensor()) {}

This comment was marked as off-topic.

@cpuhrsch
Copy link
Contributor

Instead of saying what it's not, is it feasible to say what it is? So, I guess it'd be 'getTensorTypeOpt'?

@cpuhrsch
Copy link
Contributor

cpuhrsch commented Aug 30, 2018

As with #11077 is it possible to name what it is and not what it not is?

Copy link
Contributor

@gchanan gchanan left a comment

Choose a reason for hiding this comment

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

This looks fine, or with Christian's suggestion. Or actually just fully specifying with the is_variable bool or taking TensorOptions seems simpler and we can keep the name when everything is a variable.

@ezyang
Copy link
Contributor Author

ezyang commented Aug 30, 2018

I don't want to call it 'getTensorTypeOpt' because all 'Variable's are also 'Tensor's, so it's ambiguous.

@ezyang
Copy link
Contributor Author

ezyang commented Aug 30, 2018

Re the other comment, I think the idea is that Type is not actually a public facing interface, so it doesn't matter if getType has a long and twisty name, so it's good to discourage people from using it.

Copy link
Contributor

@cpuhrsch cpuhrsch left a comment

Choose a reason for hiding this comment

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

Ok

zdevito pushed a commit to zdevito/ATen that referenced this pull request Aug 31, 2018
Summary:
Pull Request resolved: pytorch/pytorch#11077

getType now supports retrieving variable types, so make it clearer
when a getType function does NOT give you a variable type.

```
codemod -d . --extensions cc,cpp,cu,cuh,h getTypeOpt getNonVariableTypeOpt
```

Reviewed By: gchanan

Differential Revision: D9578398

fbshipit-source-id: 3ee502ac5c714849917f11ddc71de8eacfdaa9d3
PenghuiCheng pushed a commit to PenghuiCheng/pytorch that referenced this pull request Sep 11, 2018
Summary:
Pull Request resolved: pytorch#11077

getType now supports retrieving variable types, so make it clearer
when a getType function does NOT give you a variable type.

```
codemod -d . --extensions cc,cpp,cu,cuh,h getTypeOpt getNonVariableTypeOpt
```

Reviewed By: gchanan

Differential Revision: D9578398

fbshipit-source-id: 3ee502ac5c714849917f11ddc71de8eacfdaa9d3
@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

Development

Successfully merging this pull request may close these issues.

3 participants