ReshapeTransform: added missing argument in docstring by randolf-scholz · Pull Request #144401 · pytorch/pytorch

randolf-scholz · 2025-01-08T16:06:42Z

See #144197 (comment)

cc @fritzo @neerajprad @alicanb @nikitaved

pytorch-bot · 2025-01-08T16:06:46Z

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/144401

📄 Preview Python docs built from this PR
📄 Preview C++ docs built from this PR
❓ Need help or want to give feedback on the CI? Visit the bot commands wiki or our office hours

Note: Links to docs will display an error until the docs builds have been completed.

✅ No Failures

As of commit 894e6d8 with merge base 7d9f26d ():
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

randolf-scholz · 2025-01-08T16:06:53Z

@pytorchbot label "module: distributions"

randolf-scholz · 2025-01-08T16:07:24Z

@pytorchbot label "release notes: python_frontend"

randolf-scholz · 2025-01-09T10:10:57Z

Actually, what is the documentation standard? Some classes like ComposeTransform document the inherited argument, while most others do not. One should consider canonicalizing the documentation for all subclasses in transforms.py.

pytorch/torch/distributions/transforms.py

Lines 277 to 286 in 9631d1a

    
           class ComposeTransform(Transform): 
        
               """ 
        
               Composes multiple transforms in a chain. 
        
               The transforms being composed are responsible for caching. 
        
               Args: 
        
                   parts (list of :class:`Transform`): A list of transforms to compose. 
        
                   cache_size (int): Size of cache. If zero, no caching is done. If one, 
        
                       the latest single value is cached. Only 0 and 1 are supported. 
        
               """

pytorch/torch/distributions/transforms.py

Lines 402 to 414 in 9631d1a

    
           class IndependentTransform(Transform): 
        
               """ 
        
               Wrapper around another transform to treat 
        
               ``reinterpreted_batch_ndims``-many extra of the right most dimensions as 
        
               dependent. This has no effect on the forward or backward transforms, but 
        
               does sum out ``reinterpreted_batch_ndims``-many of the rightmost dimensions 
        
               in :meth:`log_abs_det_jacobian`. 
        
               Args: 
        
                   base_transform (:class:`Transform`): A base transform. 
        
                   reinterpreted_batch_ndims (int): The number of extra rightmost 
        
                       dimensions to treat as dependent. 
        
               """

pytorch/torch/distributions/transforms.py

Lines 709 to 719 in 9631d1a

    
           class AffineTransform(Transform): 
        
               r""" 
        
               Transform via the pointwise affine mapping :math:`y = \text{loc} + \text{scale} \times x`. 
        
               Args: 
        
                   loc (Tensor or float): Location parameter. 
        
                   scale (Tensor or float): Scale parameter. 
        
                   event_dim (int): Optional size of `event_shape`. This should be zero 
        
                       for univariate random variables, 1 for distributions over vectors, 
        
                       2 for distributions over matrices, etc. 
        
               """

janeyx99

nit

torch/distributions/transforms.py

janeyx99 · 2025-01-12T03:52:54Z

Regarding a standard, my inclination is that subclasses should document inherited args. Or I believe that in the end state of the docs, a reader should know all the parameters they can use. How we get there is more open ended.

Co-authored-by: Jane (Yuan) Xu <31798555+janeyx99@users.noreply.github.com>

malfet · 2025-01-13T14:46:38Z

@pytorchbot merge

pytorchmergebot · 2025-01-13T14:49:04Z

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging

Check the merge workflow status
here

…`. (#144197) Fixes #144196 Extends #144106 and #144110 ## Open Problems: - [ ] Annotating with `numbers.Number` is a bad idea, should consider using `float`, `SupportsFloat` or some `Procotol`. #144197 (comment) # Notes - `beta.py`: needed to add `type: ignore` since `broadcast_all` is untyped. - `categorical.py`: converted `else` branches of mutually exclusive arguments to `if` branch[^2]. - ~~`dirichlet.py`: replaced `axis` with `dim` arguments.~~ #144402 - `gemoetric.py`: converted `else` branches of mutually exclusive arguments to `if` branch[^2]. - ~~`independent.py`: fixed bug in `Independent.__init__` where `tuple[int, ...]` could be passed to `Distribution.__init__` instead of `torch.Size`.~~ **EDIT:** turns out the bug is related to typing of `torch.Size`. #144218 - `independent.py`: made `Independent` a generic class of its base distribution. - `multivariate_normal.py`: converted `else` branches of mutually exclusive arguments to `if` branch[^2]. - `relaxed_bernoulli.py`: added class-level type hint for `base_dist`. - `relaxed_categorical.py`: added class-level type hint for `base_dist`. - ~~`transforms.py`: Added missing argument to docstring of `ReshapeTransform`~~ #144401 - ~~`transforms.py`: Fixed bug in `AffineTransform.sign` (could return `Tensor` instead of `int`).~~ #144400 - `transforms.py`: Added `type: ignore` comments to `AffineTransform.log_abs_det_jacobian`[^1]; replaced `torch.abs(scale)` with `scale.abs()`. - `transforms.py`: Added `type: ignore` comments to `AffineTransform.__eq__`[^1]. - `transforms.py`: Fixed type hint on `CumulativeDistributionTransform.domain`. Note that this is still an LSP violation, because `Transform.domain` is defined as `Constraint`, but `Distribution.domain` is defined as `Optional[Constraint]`. - skipped: `constraints.py`, `constraints_registry.py`, `kl.py`, `utils.py`, `exp_family.py`, `__init__.py`. ## Remark `TransformedDistribution`: `__init__` uses the check `if reinterpreted_batch_ndims > 0:`, which can lead to the creation of `Independent` distributions with only 1 component. This results in awkward code like `base_dist.base_dist` in `LogisticNormal`. ```python import torch from torch.distributions import * b1 = Normal(torch.tensor([0.0]), torch.tensor([1.0])) b2 = MultivariateNormal(torch.tensor([0.0]), torch.eye(1)) t = StickBreakingTransform() d1 = TransformedDistribution(b1, t) d2 = TransformedDistribution(b2, t) print(d1.base_dist) # Independent with 1 dimension print(d2.base_dist) # MultivariateNormal ``` One could consider changing this to `if reinterpreted_batch_ndims > 1:`. [^1]: Usage of `isinstance(value, numbers.Real)` leads to problems with static typing, as the `numbers` module is not supported by `mypy` (see <python/mypy#3186>). This results in us having to add type-ignore comments in several places [^2]: Otherwise, we would have to add a bunch of `type: ignore` comments to make `mypy` happy, as it isn't able to perform the type narrowing. Ideally, such code should be replaced with structural pattern matching once support for Python 3.9 is dropped. Pull Request resolved: #144197 Approved by: https://github.com/malfet Co-authored-by: Aaron Gokaslan <aaronGokaslan@gmail.com>

…`. (pytorch#144197) Fixes pytorch#144196 Extends pytorch#144106 and pytorch#144110 ## Open Problems: - [ ] Annotating with `numbers.Number` is a bad idea, should consider using `float`, `SupportsFloat` or some `Procotol`. pytorch#144197 (comment) # Notes - `beta.py`: needed to add `type: ignore` since `broadcast_all` is untyped. - `categorical.py`: converted `else` branches of mutually exclusive arguments to `if` branch[^2]. - ~~`dirichlet.py`: replaced `axis` with `dim` arguments.~~ pytorch#144402 - `gemoetric.py`: converted `else` branches of mutually exclusive arguments to `if` branch[^2]. - ~~`independent.py`: fixed bug in `Independent.__init__` where `tuple[int, ...]` could be passed to `Distribution.__init__` instead of `torch.Size`.~~ **EDIT:** turns out the bug is related to typing of `torch.Size`. pytorch#144218 - `independent.py`: made `Independent` a generic class of its base distribution. - `multivariate_normal.py`: converted `else` branches of mutually exclusive arguments to `if` branch[^2]. - `relaxed_bernoulli.py`: added class-level type hint for `base_dist`. - `relaxed_categorical.py`: added class-level type hint for `base_dist`. - ~~`transforms.py`: Added missing argument to docstring of `ReshapeTransform`~~ pytorch#144401 - ~~`transforms.py`: Fixed bug in `AffineTransform.sign` (could return `Tensor` instead of `int`).~~ pytorch#144400 - `transforms.py`: Added `type: ignore` comments to `AffineTransform.log_abs_det_jacobian`[^1]; replaced `torch.abs(scale)` with `scale.abs()`. - `transforms.py`: Added `type: ignore` comments to `AffineTransform.__eq__`[^1]. - `transforms.py`: Fixed type hint on `CumulativeDistributionTransform.domain`. Note that this is still an LSP violation, because `Transform.domain` is defined as `Constraint`, but `Distribution.domain` is defined as `Optional[Constraint]`. - skipped: `constraints.py`, `constraints_registry.py`, `kl.py`, `utils.py`, `exp_family.py`, `__init__.py`. ## Remark `TransformedDistribution`: `__init__` uses the check `if reinterpreted_batch_ndims > 0:`, which can lead to the creation of `Independent` distributions with only 1 component. This results in awkward code like `base_dist.base_dist` in `LogisticNormal`. ```python import torch from torch.distributions import * b1 = Normal(torch.tensor([0.0]), torch.tensor([1.0])) b2 = MultivariateNormal(torch.tensor([0.0]), torch.eye(1)) t = StickBreakingTransform() d1 = TransformedDistribution(b1, t) d2 = TransformedDistribution(b2, t) print(d1.base_dist) # Independent with 1 dimension print(d2.base_dist) # MultivariateNormal ``` One could consider changing this to `if reinterpreted_batch_ndims > 1:`. [^1]: Usage of `isinstance(value, numbers.Real)` leads to problems with static typing, as the `numbers` module is not supported by `mypy` (see <python/mypy#3186>). This results in us having to add type-ignore comments in several places [^2]: Otherwise, we would have to add a bunch of `type: ignore` comments to make `mypy` happy, as it isn't able to perform the type narrowing. Ideally, such code should be replaced with structural pattern matching once support for Python 3.9 is dropped. Pull Request resolved: pytorch#144197 Approved by: https://github.com/malfet Co-authored-by: Aaron Gokaslan <aaronGokaslan@gmail.com>

ReshapeTransform: added missing argument in docstring

13ca369

pytorch-bot bot added the module: distributions Related to torch.distributions label Jan 8, 2025

pytorch-bot bot added the release notes: python_frontend python frontend release notes category label Jan 8, 2025

randolf-scholz mentioned this pull request Jan 8, 2025

[typing] Add type hints to __init__ methods in torch.distributions. #144197

Closed

1 task

pytorchbot added the open source label Jan 8, 2025

janeyx99 approved these changes Jan 12, 2025

View reviewed changes

torch/distributions/transforms.py Outdated Show resolved Hide resolved

janeyx99 added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Jan 12, 2025

malfet approved these changes Jan 13, 2025

View reviewed changes

Update torch/distributions/transforms.py

894e6d8

Co-authored-by: Jane (Yuan) Xu <31798555+janeyx99@users.noreply.github.com>

pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Jan 13, 2025

pytorchmergebot added the merging label Jan 13, 2025

pytorchmergebot added the Merged label Jan 13, 2025

pytorchmergebot closed this in 983bf60 Jan 13, 2025

pytorchmergebot removed the merging label Jan 13, 2025

randolf-scholz deleted the fix_reshapetransform_docstring branch January 28, 2025 09:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ReshapeTransform: added missing argument in docstring#144401

ReshapeTransform: added missing argument in docstring#144401
randolf-scholz wants to merge 2 commits intopytorch:mainfrom
randolf-scholz:fix_reshapetransform_docstring

randolf-scholz commented Jan 8, 2025 •

edited by pytorch-bot bot

Loading

Uh oh!

pytorch-bot bot commented Jan 8, 2025 •

edited

Loading

Uh oh!

randolf-scholz commented Jan 8, 2025

Uh oh!

randolf-scholz commented Jan 8, 2025

Uh oh!

randolf-scholz commented Jan 9, 2025 •

edited

Loading

Uh oh!

janeyx99 left a comment

Uh oh!

Uh oh!

janeyx99 commented Jan 12, 2025

Uh oh!

malfet commented Jan 13, 2025

Uh oh!

pytorchmergebot commented Jan 13, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

randolf-scholz commented Jan 8, 2025 • edited by pytorch-bot bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pytorch-bot bot commented Jan 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/144401

✅ No Failures

Uh oh!

randolf-scholz commented Jan 8, 2025

Uh oh!

randolf-scholz commented Jan 8, 2025

Uh oh!

randolf-scholz commented Jan 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

janeyx99 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

janeyx99 commented Jan 12, 2025

Uh oh!

malfet commented Jan 13, 2025

Uh oh!

pytorchmergebot commented Jan 13, 2025

Merge started

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

randolf-scholz commented Jan 8, 2025 •

edited by pytorch-bot bot

Loading

pytorch-bot bot commented Jan 8, 2025 •

edited

Loading

randolf-scholz commented Jan 9, 2025 •

edited

Loading