[fix] torch.{lin, log}space(): properly examine passed dtype #53685

kshitij12345 · 2021-03-10T05:41:35Z

facebook-github-bot · 2021-03-10T05:41:48Z

💊 CI failures summary and remediations

As of commit 4bc39f0 (more details on the Dr. CI page):

1/1 failures introduced in this PR

1 failure not recognized by patterns:

Job	Step	Action
^{Windows CI (pytorch-win-vs2019-cpu-py3) / render_test_results}	^{Set up job}	🔁 rerun

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.

Click here to manually regenerate this comment.

kshitij12345 · 2021-03-31T09:38:26Z

tools/autograd/templates/python_torch_functions.cpp

+  if (_r.isNone(4)) {
+    // aten::logspace(Scalar start, Scalar end, int? steps=None, float base=10.0, *, ScalarType? dtype=None, Layout? layout=None, Device? device=None, bool? pin_memory=None) -> Tensor
+    const auto options = TensorOptions()
+        .dtype(_r.scalartypeOptional(5))


Everything is same here w.r.t generated code except for _r.scalartypeOptional(5) instead of _r.scalartype(5)

Ditto re explanatory comment

kshitij12345 · 2021-03-31T09:38:38Z

tools/autograd/templates/python_torch_functions.cpp

+  if (_r.isNone(3)) {
+    // aten::linspace(Scalar start, Scalar end, int? steps=None, *, ScalarType? dtype=None, Layout? layout=None, Device? device=None, bool? pin_memory=None) -> Tensor
+    const auto options = TensorOptions()
+        .dtype(_r.scalartypeOptional(4))


Everything is same here w.r.t generated code except for _r.scalartypeOptional(5) instead of _r.scalartype(5)

Might want to add a comment a la https://github.com/pytorch/pytorch/blob/master/tools/autograd/templates/python_torch_functions.cpp#L104 - helps explain why this needs to be a manual binding

mruberry · 2021-04-05T07:06:12Z

@anjali411 Would you take the lead on reviewing this?

anjali411 · 2021-05-10T19:19:24Z

@kshitij12345 sorry I missed the notification for this PR before. This PR looks great overall. ~~We should probably set the manual_cpp_binding flag to True right?~~ edit -- this is not correct

anjali411 · 2021-05-10T19:28:55Z

aten/src/ATen/native/TensorFactories.cpp

  const auto steps_ = steps.value_or(100);
  TORCH_CHECK(steps_ >= 0, "number of steps must be non-negative");
-  auto result_options = linspace_logspace_infer_options(start, end, options);
+  auto result_options = linspace_logspace_infer_options(start, end, options, "torch.linspace()");


@kshitij12345 so if the user doesn't specify the dtype, the dtype is not set until here, or the line below in which case we create a tensor with the default dtype?

Apologies missed the comment. Thanks @bhosmer for pointing out. Have updated linspace_logspace_infer_options.

facebook-github-bot · 2021-05-10T19:30:38Z

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

aten/src/ATen/native/TensorFactories.cpp

mruberry · 2021-05-11T05:37:36Z

tools/autograd/templates/python_torch_functions.cpp

 static PyObject * THPVariable_numel(PyObject* self_, PyObject* args, PyObject* kwargs);

+// linspace
+static PyObject * THPVariable_linspace(PyObject* self_, PyObject* args, PyObject* kwargs)


@bhosmer would you like to check these custom bindings?

mruberry · 2021-05-11T05:39:29Z

Hey @kshitij12345, this looks correct but I'll need more time to review the new handwritten bindings; I cc'd @bhosmer to see if he'd like to take a look or suggest an alternative approach, too

With the branch cut I'll need some extra time to review the new bindings carefully; would you ping me on this next week? I hate to make you wait but it's an incredibly busy time

mruberry · 2021-05-11T05:41:18Z

Linking with #56335, which also updates lin and log space

kshitij12345 · 2021-05-11T05:50:28Z

@mruberry Sure. I understand. Will ping you by next week here!

@mruberry @anjali411 Thanks for looking into it :)

bhosmer

Hey @mruberry @anjali411 @kshitij12345 - yeah, unfortunately the codegen still locks you into the dtype defaulting behavior for ops that take TensorOptions args, so manual bindings are the way to make it work for now. Couple small suggestions inline, otherwise LGTM

bhosmer · 2021-05-12T20:46:34Z

tools/autograd/templates/python_torch_functions.cpp

+  if (_r.isNone(3)) {
+    // aten::linspace(Scalar start, Scalar end, int? steps=None, *, ScalarType? dtype=None, Layout? layout=None, Device? device=None, bool? pin_memory=None) -> Tensor
+    const auto options = TensorOptions()
+        .dtype(_r.scalartypeOptional(4))


Might want to add a comment a la https://github.com/pytorch/pytorch/blob/master/tools/autograd/templates/python_torch_functions.cpp#L104 - helps explain why this needs to be a manual binding

bhosmer · 2021-05-12T20:47:07Z

tools/autograd/templates/python_torch_functions.cpp

+  if (_r.isNone(4)) {
+    // aten::logspace(Scalar start, Scalar end, int? steps=None, float base=10.0, *, ScalarType? dtype=None, Layout? layout=None, Device? device=None, bool? pin_memory=None) -> Tensor
+    const auto options = TensorOptions()
+        .dtype(_r.scalartypeOptional(5))


Ditto re explanatory comment

bhosmer · 2021-05-12T20:57:51Z

aten/src/ATen/native/TensorFactories.cpp

  }

-  return result_options;
+  return options;


Per @anjali411's comment below, the other change introduced here besides the error checking is that the options we return will no longer have the default dtype patched in, in the non-complex case.

It may be that the current downstream consumer (empty) happens to implement the same defaulting behavior atm, but I think it'll be more robust if we don't depend on that, and instead do it here.

Updated. Thanks!!

codecov · 2021-05-13T14:38:40Z

Codecov Report

Merging #53685 (6215555) into master (6ee9466) will increase coverage by 0.36%.
The diff coverage is 100.00%.

❗ Current head 6215555 differs from pull request most recent head 3cdac1f. Consider uploading reports for the commit 3cdac1f to get more accurate results

@@            Coverage Diff             @@
##           master   #53685      +/-   ##
==========================================
+ Coverage   76.44%   76.80%   +0.36%     
==========================================
  Files        2022     1986      -36     
  Lines      202375   198202    -4173     
==========================================
- Hits       154701   152237    -2464     
+ Misses      47674    45965    -1709

kshitij12345 · 2021-05-20T09:40:47Z

would you ping me on this next week?
@mruberry gentle ping :)

kshitij12345 · 2021-05-26T18:01:02Z

@mruberry Gentle ping :)

tools/autograd/templates/python_torch_functions.cpp

anjali411

LGTM! Thanks @kshitij12345 this is ready for land once you rebase it and update the comment for more context! :D

facebook-github-bot · 2021-06-08T17:46:51Z

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

…ogspace-passed-dtype

kshitij12345 · 2021-06-10T03:07:10Z

@anjali411 gentle ping :)

facebook-github-bot · 2021-06-10T14:21:48Z

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

facebook-github-bot · 2021-06-10T19:02:12Z

@anjali411 merged this pull request in c902609.

torch.{lin, log}space(): properly examine passed dtype

54523eb

facebook-github-bot added the cla signed label Mar 10, 2021

pytorchbot added the open source label Mar 10, 2021

Merge branch 'master' into fix/linspace-logspace-passed-dtype

219747e

kshitij12345 commented Mar 31, 2021

View reviewed changes

kshitij12345 marked this pull request as ready for review March 31, 2021 09:38

kshitij12345 requested review from albanD and soulitzer as code owners March 31, 2021 09:38

kshitij12345 requested review from anjali411 and mruberry and removed request for albanD and soulitzer March 31, 2021 09:39

gchanan added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Mar 31, 2021

anjali411 reviewed May 10, 2021

View reviewed changes

Merge branch 'master' into fix/linspace-logspace-passed-dtype

189036d

mruberry reviewed May 11, 2021

View reviewed changes

aten/src/ATen/native/TensorFactories.cpp Outdated Show resolved Hide resolved

mruberry reviewed May 11, 2021

View reviewed changes

aten/src/ATen/native/TensorFactories.cpp Outdated Show resolved Hide resolved

mruberry reviewed May 11, 2021

View reviewed changes

address review and minor changes

4bee0b0

make mypy happy

b6b3d46

bhosmer reviewed May 12, 2021

View reviewed changes

Merge branch 'master' into fix/linspace-logspace-passed-dtype

949537c

kshitij12345 added 2 commits May 13, 2021 03:34

address review

0d4b45c

add relevant entry for mypy signature

6215555

Merge branch 'pytorch:master' into fix/linspace-logspace-passed-dtype

3cdac1f

anjali411 reviewed Jun 8, 2021

View reviewed changes

tools/autograd/templates/python_torch_functions.cpp Show resolved Hide resolved

anjali411 approved these changes Jun 8, 2021

View reviewed changes

anjali411 added the module: complex Related to complex number support in PyTorch label Jun 8, 2021

kshitij12345 added 6 commits June 8, 2021 14:44

Merge branch 'master' into fix/linspace-logspace-passed-dtype

0c4c201

add comment

0df3431

Merge branch 'fix/linspace-logspace-passed-dtype' into fix/linspace-l…

82e1b03

…ogspace-passed-dtype

Merge branch 'pytorch:master' into fix/linspace-logspace-passed-dtype

6ff2f2e

Merge branch 'pytorch:master' into fix/linspace-logspace-passed-dtype

d07339f

Merge branch 'pytorch:master' into fix/linspace-logspace-passed-dtype

4bc39f0

facebook-github-bot closed this in c902609 Jun 10, 2021

facebook-github-bot added the Merged label Jun 10, 2021

[fix] torch.{lin, log}space(): properly examine passed dtype #53685

[fix] torch.{lin, log}space(): properly examine passed dtype #53685

Uh oh!

Conversation

kshitij12345 commented Mar 10, 2021

Uh oh!

facebook-github-bot commented Mar 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💊 CI failures summary and remediations

1 failure not recognized by patterns:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mruberry commented Apr 5, 2021

Uh oh!

anjali411 commented May 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

anjali411 May 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented May 10, 2021

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mruberry commented May 11, 2021

Uh oh!

mruberry commented May 11, 2021

Uh oh!

kshitij12345 commented May 11, 2021

Uh oh!

bhosmer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented May 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

kshitij12345 commented May 20, 2021

Uh oh!

kshitij12345 commented May 26, 2021

Uh oh!

Uh oh!

anjali411 left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Jun 8, 2021

Uh oh!

kshitij12345 commented Jun 10, 2021

Uh oh!

facebook-github-bot commented Jun 10, 2021

Uh oh!

facebook-github-bot commented Jun 10, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

facebook-github-bot commented Mar 10, 2021 •

edited

Loading

anjali411 commented May 10, 2021 •

edited

Loading

anjali411 May 10, 2021 •

edited

Loading

codecov bot commented May 13, 2021 •

edited

Loading