KEMBAR78
[inductor] Added decomposition for upsample_nearest_exact Nd by vfdev-5 · Pull Request #113749 · pytorch/pytorch · GitHub
Skip to content

Conversation

@vfdev-5
Copy link
Contributor

@vfdev-5 vfdev-5 commented Nov 15, 2023

@pytorch-bot
Copy link

pytorch-bot bot commented Nov 15, 2023

🔗 Helpful Links

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

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

✅ No Failures

As of commit a7cf034 with merge base 115da02 (image):
💚 Looks good so far! There are no failures yet. 💚

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

@vfdev-5 vfdev-5 marked this pull request as ready for review November 16, 2023 08:44
@vfdev-5 vfdev-5 added the topic: not user facing topic category label Nov 17, 2023
@janeyx99
Copy link
Contributor

@voznesenskym added you as reviewer due to the inductor/lowering, but please reassign if there is a better reviewer

@janeyx99 janeyx99 added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Nov 17, 2023
@vfdev-5
Copy link
Contributor Author

vfdev-5 commented Nov 21, 2023

@pytorchbot merge

@pytorch-bot pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Nov 21, 2023
@pytorchmergebot
Copy link
Collaborator

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

@vfdev-5 vfdev-5 deleted the decomp-add-upsample-nearest-exact branch November 21, 2023 13:27
pytorchmergebot pushed a commit that referenced this pull request Jan 17, 2024
Fixed #116848

Related to the bug introduced in my previous PR here: https://github.com/pytorch/pytorch/pull/113749/files#diff-a1b077971cddfabfa0071c5162265066e867bc07721816d95b9cbe58431c38e3R3264

Originally, the code was
```python
def upsample_nearestnd(
    x,
    output_size,
    scales_x: Tuple[Optional[float], ...],
    n: int = 2,
    exact: bool = False,
):
   # ...
    scales = [i / o for i, o in zip(i_sizes, o_sizes)]
    for i, scale in enumerate(scales):
        if scale:
            scales[i] = scale
```
which is wrong as `scales_x` is not used but can be provided by the user. The code was working for cases when user provided scale value can be recomputed using `input / output` sizes, e.g. scale=2.0. However, this would fail if input scale is a float value, e.g. 2.3, in this case recomputed scale is a bit different (e.g. 2.292682926829268, depending on input and output size) and can lead to an inconsistent output.
This problem was "fixed" to the following in my previous PR: #113749
```python
def upsample_nearestnd(
    x,
    output_size,
    scales_x: Tuple[Optional[float], ...],
    n: int = 2,
    exact: bool = False,
):
   # ...
    scales = [i / o for i, o in zip(i_sizes, o_sizes)]
    for i, scale in enumerate(scales_x):
        if scale:
            scales[i] = scale
```
however, this leads to a wrong scale value as it should be inverted as (1 / scale).

Pull Request resolved: #117538
Approved by: https://github.com/peterbell10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ciflow/inductor ciflow/trunk Trigger trunk jobs on your pull request Merged module: inductor open source topic: not user facing topic category triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants