Fix typing errors in the torch.distributions module #45689

xuzhao9 · 2020-10-01T19:23:10Z

walterddr · 2020-10-05T17:39:20Z

mypy.ini

+[mypy-torch.jit.quantized]
+ignore_errors = True
+
+[mypy-torch.nn.functional]


are these actually intended to be added? or is this a merge master issue?

They are added because after annotating the distributions.* dir, a lot of typing errors appear under these two modules. Maybe it's because master update. I am looking into this.

walterddr · 2020-10-05T17:40:55Z

torch/distributions/distribution.py

@@ -71,7 +72,7 @@ def event_shape(self):
        """
        return self._event_shape

-    @property
+    @property  # type: ignore[no-redef]


do we have a mypy git issue link? (is it: python/mypy#6185?)

do we have a mypy git issue link? (is it: python/mypy#6185?)

No it is not - this is simply because the name "arg_constraints" is reused as a member variable and a method name at the same time. I don't find a related mypy git issue. This could be resolved by renaming one of them, but that would introduce too much code change IMO.

it would be nice to put a comment to explain this

Hmm, if arg_constraints is an internal variable, perhaps an underscore prefix should be added to it? ( Which could be done as a separate PR)

walterddr · 2020-10-05T17:41:58Z

torch/distributions/multinomial.py

@@ -2,7 +2,7 @@
 from torch._six import inf
 from torch.distributions.distribution import Distribution
 from torch.distributions import Categorical
-from numbers import Number
+import numbers


seems like only Integral was used? should we just keep original format?

Suggested change

import numbers

from numbers import Integral

seems like only Integral was used? should we just keep original format?

Nice catch. Will update.

walterddr · 2020-10-05T17:54:03Z

torch/distributions/multinomial.py

@@ -88,7 +89,7 @@ def param_shape(self):

    def sample(self, sample_shape=torch.Size()):
        sample_shape = torch.Size(sample_shape)
-        samples = self._categorical.sample(torch.Size((self.total_count,)) + sample_shape)
+        samples = self._categorical.sample(torch.Size((int(self.total_count),).__iter__()) + sample_shape)


I was wondering if we can remove __iter__() seems like (int(val),) should return a Tuple[int] type. yes?

You are right! I have removed it.

That's correct! Will update

walterddr · 2020-10-05T17:54:53Z

torch/distributions/utils.py

@@ -94,7 +96,7 @@ class lazy_property(object):
    """
    def __init__(self, wrapped):
        self.wrapped = wrapped
-        update_wrapper(self, wrapped)
+        update_wrapper(self, wrapped)  # type: ignore[arg-type]


is there a link to the reason?

Fixing this error requires annotating a built-in python package(functools), therefore I just ignore this error.
More details: update_wrapper(wrapper: [Callable, ...], wrapped) is a functools function. Here the Argument 1 is "self", which is "class lazy_property", therefore the type mismatches.

To fix this problem we need to annotate the update_wrapper(wrapper, wrapped), which is in a third-party python package(functools). The function's first parameter is "[Callable, ...]" whereas the Argument 1 here("self") is type "class lazy_property(object)".

walterddr

thanks for the update.

also i think you might've rebased incorrectly with all those third-party changes. could you use the instruction @janeyx99 just added here: #45903 to update your PR

walterddr · 2020-10-07T01:05:42Z

torch/distributions/distribution.py

@@ -71,7 +72,7 @@ def event_shape(self):
        """
        return self._event_shape

-    @property
+    @property  # type: ignore[no-redef]


it would be nice to put a comment to explain this

walterddr · 2020-10-07T01:06:31Z

torch/distributions/normal.py

@@ -1,4 +1,5 @@
 import math
+import numbers


same here

Suggested change

import numbers

from numbers import Real

walterddr · 2020-10-07T01:06:41Z

torch/distributions/beta.py

@@ -1,3 +1,4 @@
+import numbers


same here

Suggested change

import numbers

from numbers import Real

walterddr

lgtm

codecov · 2020-10-07T22:23:37Z

Codecov Report

Merging #45689 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master   #45689      +/-   ##
==========================================
+ Coverage   68.25%   68.26%   +0.01%     
==========================================
  Files         410      410              
  Lines       53246    53266      +20     
==========================================
+ Hits        36344    36363      +19     
- Misses      16902    16903       +1

Impacted Files	Coverage Δ
torch/distributions/__init__.py	`100.00% <100.00%> (ø)`
torch/distributions/beta.py	`96.49% <100.00%> (ø)`
torch/distributions/distribution.py	`90.72% <100.00%> (ø)`
torch/distributions/independent.py	`91.37% <100.00%> (+0.15%)`	⬆️
torch/distributions/kl.py	`97.52% <100.00%> (+<0.01%)`	⬆️
torch/distributions/mixture_same_family.py	`95.74% <100.00%> (+0.04%)`	⬆️
torch/distributions/multinomial.py	`95.31% <100.00%> (ø)`
torch/distributions/normal.py	`96.87% <100.00%> (+0.04%)`	⬆️
torch/distributions/transformed_distribution.py	`97.53% <100.00%> (+0.03%)`	⬆️
torch/distributions/transforms.py	`94.34% <100.00%> (+0.15%)`	⬆️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9679e1a...3928b46. Read the comment docs.

malfet · 2020-10-08T19:45:40Z

torch/distributions/__init__.py

@@ -155,4 +156,4 @@
    'register_kl',
    'transform_to',
 ]
-__all__.extend(transforms.__all__)
+__all__.extend(transform_all)


Can you please add a comment, why this is needed_

torch/distributions/beta.py

malfet · 2020-10-08T19:50:28Z

torch/distributions/distribution.py

@@ -71,7 +72,7 @@ def event_shape(self):
        """
        return self._event_shape

-    @property
+    @property  # type: ignore[no-redef]


Hmm, if arg_constraints is an internal variable, perhaps an underscore prefix should be added to it? ( Which could be done as a separate PR)

malfet · 2020-10-08T19:51:06Z

torch/distributions/distribution.py

@@ -81,7 +83,8 @@ def arg_constraints(self):
        """
        raise NotImplementedError

-    @property
+    # Ignore the mypy type error caused by redefining `support` as a method
+    @property  # type: ignore[no-redef]


Same as above, perhaps the variable should be renamed to something else.

malfet · 2020-10-08T19:55:17Z

torch/distributions/transforms.py

@@ -500,8 +511,8 @@ def __eq__(self, other):

    @property
    def sign(self):
-        if isinstance(self.scale, numbers.Number):
-            return 1 if self.scale > 0 else -1 if self.scale < 0 else 0
+        if isinstance(self.scale, numbers.Integral):


Hmm, changing Number to Integral here would lead to a crash if scale is float, right? (because there are no sign method for the float type, is there?

malfet · 2020-10-08T22:03:50Z

torch/distributions/multinomial.py

@@ -50,7 +51,7 @@ def variance(self):
        return self.total_count * self.probs * (1 - self.probs)

    def __init__(self, total_count=1, probs=None, logits=None, validate_args=None):
-        if not isinstance(total_count, Number):
+        if not isinstance(total_count, Integral):


Are you sure that total_count must be an integral value?

malfet · 2020-10-08T22:04:49Z

torch/distributions/multinomial.py

@@ -40,6 +40,7 @@ class Multinomial(Distribution):
    """
    arg_constraints = {'probs': constraints.simplex,
                       'logits': constraints.real}
+    total_count: Integral


Why this one can not be just int?

Suggested change

total_count: Integral

total_count: int

dr-ci · 2020-10-09T03:53:42Z

💊 CI failures summary and remediations

As of commit 3928b46 (more details on the Dr. CI page):

💚 💚 Looks good so far! There are no failures yet. 💚 💚

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 on the GitHub issue tracker or post in the (internal) Dr. CI Users group.

See how this bot performed.

This comment has been revised 7 times.

facebook-github-bot

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

…#42979)

facebook-github-bot

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

facebook-github-bot · 2020-10-12T18:15:04Z

@xuzhao9 merged this pull request in 146721f.

Summary: Fixes hiding of the `CatTransform` docstring due to a misplaced type annotation in #45689. Also fixes that annotation and adds to to `StackTransform` to keep `CatTransform` similar. Pull Request resolved: #73747 Reviewed By: mruberry Differential Revision: D34649927 Pulled By: neerajprad fbshipit-source-id: e4594fd76020a37ac4cada88e5fb08191984e911

Summary: Fixes hiding of the `CatTransform` docstring due to a misplaced type annotation in #45689. Also fixes that annotation and adds to to `StackTransform` to keep `CatTransform` similar. Pull Request resolved: #73747 Reviewed By: mruberry Differential Revision: D34649927 Pulled By: neerajprad fbshipit-source-id: e4594fd76020a37ac4cada88e5fb08191984e911 (cherry picked from commit cec256c)

Summary: Fixes hiding of the `CatTransform` docstring due to a misplaced type annotation in pytorch/pytorch#45689. Also fixes that annotation and adds to to `StackTransform` to keep `CatTransform` similar. Pull Request resolved: pytorch/pytorch#73747 Reviewed By: mruberry Differential Revision: D34649927 Pulled By: neerajprad fbshipit-source-id: e4594fd76020a37ac4cada88e5fb08191984e911 (cherry picked from commit cec256c3242d1cf55073a980060af87c1fd59ac9)

xuzhao9 requested a review from malfet October 1, 2020 19:23

xuzhao9 self-assigned this Oct 1, 2020

xuzhao9 added module: typing Related to mypy type annotations module: distributions Related to torch.distributions labels Oct 1, 2020

xuzhao9 requested review from samestep and walterddr October 5, 2020 16:55

walterddr reviewed Oct 5, 2020

View reviewed changes

xuzhao9 requested a review from walterddr October 5, 2020 21:18

walterddr reviewed Oct 7, 2020

View reviewed changes

xuzhao9 force-pushed the xzhao9/distributions-typing branch from 182a3a2 to f1e9efc Compare October 7, 2020 18:17

xuzhao9 requested a review from walterddr October 7, 2020 18:28

walterddr approved these changes Oct 7, 2020

View reviewed changes

malfet reviewed Oct 8, 2020

View reviewed changes

xuzhao9 force-pushed the xzhao9/distributions-typing branch from f1e9efc to 2e718f1 Compare October 8, 2020 20:53

xuzhao9 requested a review from malfet October 8, 2020 20:56

malfet approved these changes Oct 8, 2020

View reviewed changes

xuzhao9 force-pushed the xzhao9/distributions-typing branch 3 times, most recently from f32e6de to 47cb358 Compare October 9, 2020 03:30

facebook-github-bot reviewed Oct 9, 2020

View reviewed changes

Fix mypy typing errors in module torch.distributions.* (issue pytorch…

3928b46

…#42979)

xuzhao9 force-pushed the xzhao9/distributions-typing branch 2 times, most recently from 6ff4772 to 3928b46 Compare October 9, 2020 22:25

facebook-github-bot reviewed Oct 9, 2020

View reviewed changes

facebook-github-bot closed this in 146721f Oct 12, 2020

xuzhao9 deleted the xzhao9/distributions-typing branch October 12, 2020 17:46

facebook-github-bot added the Merged label Oct 12, 2020

fritzo mentioned this pull request Mar 3, 2022

Fix docstring hiding due to #45689 #73747

Closed

WBobby mentioned this pull request Aug 17, 2022

Add ROCm5.2.3/AMDGPU support for PyTorch WBobby/pytorch#2

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix typing errors in the torch.distributions module #45689

Fix typing errors in the torch.distributions module #45689

xuzhao9 commented Oct 1, 2020 •

edited

Loading

walterddr Oct 5, 2020

xuzhao9 Oct 5, 2020

walterddr Oct 5, 2020

xuzhao9 Oct 5, 2020

walterddr Oct 7, 2020

malfet Oct 8, 2020

walterddr Oct 5, 2020

xuzhao9 Oct 5, 2020

walterddr Oct 5, 2020

xuzhao9 Oct 5, 2020

xuzhao9 Oct 5, 2020

walterddr Oct 5, 2020

xuzhao9 Oct 5, 2020

xuzhao9 Oct 5, 2020

walterddr left a comment •

edited

Loading

walterddr Oct 7, 2020

walterddr Oct 7, 2020

walterddr Oct 7, 2020

walterddr left a comment

codecov bot commented Oct 7, 2020 •

edited

Loading

malfet Oct 8, 2020

malfet Oct 8, 2020

malfet Oct 8, 2020

malfet Oct 8, 2020

malfet Oct 8, 2020

malfet Oct 8, 2020

dr-ci bot commented Oct 9, 2020 •

edited

Loading

facebook-github-bot left a comment

facebook-github-bot left a comment

facebook-github-bot commented Oct 12, 2020

Fix typing errors in the torch.distributions module #45689

Fix typing errors in the torch.distributions module #45689

Conversation

xuzhao9 commented Oct 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

walterddr left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

walterddr left a comment

Choose a reason for hiding this comment

codecov bot commented Oct 7, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dr-ci bot commented Oct 9, 2020 • edited Loading

💊 CI failures summary and remediations

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Oct 12, 2020

xuzhao9 commented Oct 1, 2020 •

edited

Loading

walterddr left a comment •

edited

Loading

codecov bot commented Oct 7, 2020 •

edited

Loading

dr-ci bot commented Oct 9, 2020 •

edited

Loading