Skip to content

Don't allow requires_grad to be set on integer Tensor constructors in…#7185

Merged
gchanan merged 7 commits intopytorch:masterfrom
gchanan:requires_grad_new_error
May 18, 2018
Merged

Don't allow requires_grad to be set on integer Tensor constructors in…#7185
gchanan merged 7 commits intopytorch:masterfrom
gchanan:requires_grad_new_error

Conversation

@gchanan
Copy link
Contributor

@gchanan gchanan commented May 2, 2018

… tensor_new.

@gchanan gchanan requested a review from colesbury May 2, 2018 18:11
@onnxbot-worker-1 onnxbot-worker-1 mentioned this pull request May 2, 2018
Closed
@gchanan
Copy link
Contributor Author

gchanan commented May 2, 2018

This completes #7034; I would argue this should perhaps be a (python) warning rather than an error (see discussion in #7034), but let's make it consistent for now.

@gchanan
Copy link
Contributor Author

gchanan commented May 2, 2018

@pytorchbot retest this please.

goldsborough
goldsborough reviewed May 3, 2018
View reviewed changes
torch/csrc/autograd/utils/python_variables.h

namespace torch { namespace autograd { namespace utils {

inline at::Tensor set_requires_grad(at::Tensor self, bool requires_grad) {

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

@gchanan
Copy link
Contributor Author

gchanan commented May 3, 2018

#7034 also didn't put this error Variable(..., requires_grad=True)

@ezyang
Copy link
Contributor

ezyang commented May 13, 2018 

6:09:33 ======================================================================
16:09:33 ERROR: test_cdf_log_prob (test_distributions.TestDistributions)
16:09:33 ----------------------------------------------------------------------
16:09:33 Traceback (most recent call last):
16:09:33   File "/var/lib/jenkins/workspace/test/test_distributions.py", line 1871, in test_cdf_log_prob
16:09:33     samples = torch.tensor(dist.sample().data, requires_grad=True)
16:09:33 RuntimeError: only Tensors of floating point dtype can require gradients

@gchanan gchanan merged commit 7abdc30 into pytorch:master May 18, 2018
onnxbot added a commit to onnxbot/onnx-fb-universe that referenced this pull request May 18, 2018
@onnxbot
[auto] Update pytorch to 7abdc30 - Don't allow requires_grad to be se…
981c26c 
…t on integer Tensor constructors in… (pytorch/pytorch#7185)

pytorch/pytorch@7abdc30
weiyangfb pushed a commit to weiyangfb/pytorch that referenced this pull request Jun 11, 2018
@gchanan @weiyangfb
Don't allow requires_grad to be set on integer Tensor constructors in… (
14e7482 
pytorch#7185)

* Don't allow requires_grad to be set on integer Tensor constructors in tensor_new.

* Fix autograd test.

* Fix test_distributions.

* Fix test_jit.

* Fix NN tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ezyang ezyang ezyang approved these changes

@colesbury colesbury Awaiting requested review from colesbury

@apaszke apaszke Awaiting requested review from apaszke

@soumith soumith Awaiting requested review from soumith

@zdevito zdevito Awaiting requested review from zdevito

+1 more reviewer

@goldsborough goldsborough goldsborough left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@gchanan @ezyang @apaszke @goldsborough