Skip to content

Recoded _max_value method using a dictionary #5566

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Mar 9, 2022

Conversation

vfdev-5
Copy link
Collaborator

@vfdev-5 vfdev-5 commented Mar 8, 2022

Description:

  • Removed _max_value method and added a dictionary

Related to #5502

cc @datumbox

@facebook-github-bot
Copy link

facebook-github-bot commented Mar 8, 2022

💊 CI failures summary and remediations

As of commit 93a7905 (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).

Please report bugs/suggestions to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

Copy link
Contributor

@datumbox datumbox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @vfdev-5. A few comments + there are related failing tests.

@vfdev-5 vfdev-5 changed the title Removed _max_value method and added a dictionary Recoded _max_value method using a dictionary Mar 8, 2022
Copy link
Contributor

@datumbox datumbox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks! One last nit for your consideration:

@pmeier
Copy link
Collaborator

pmeier commented Mar 9, 2022

We could probably also add an xfail test like

import torch
import pytest


@pytest.mark.xfail(
    reason="torch.iinfo() is not supported by torchscript. See https://github.com/pytorch/pytorch/issues/41492."
)
def test_max_value_iinfo():
    @torch.jit.script
    def max_value(image: torch.Tensor) -> int:
        return 1 if image.is_floating_point() else torch.iinfo(image.dtype).max

to nudge us to switch as soon as support is added.

@datumbox
Copy link
Contributor

datumbox commented Mar 9, 2022

@pmeier Good call. If they add support our test will fail and we will certainly know about it :)

@vfdev-5 Feel free to merge on green CI.

@vfdev-5 vfdev-5 merged commit 57c8de7 into pytorch:main Mar 9, 2022
@vfdev-5 vfdev-5 deleted the fix_max_values branch March 9, 2022 11:49
elif dtype == torch.int64:
return int(2 ** 63) - 1
else:
return 1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JIT on internal tests REALLY doesn't like this and fails with:

runtime error: 9.22337e+18 is outside the range of representable values of type 'long'

Replacing this with the following works:

def _max_value(dtype: torch.dtype) -> int:
    if dtype == torch.uint8:
        return 255
    elif dtype == torch.int8:
        return 127
    elif dtype == torch.int16:
        return 32767
    elif dtype == torch.int32:
        return 2147483647
    elif dtype == torch.int64:
        return 9223372036854775807
    else:
        return 1

I'll cherrypick it after it's merged internally.

facebook-github-bot pushed a commit that referenced this pull request Mar 15, 2022
Summary:
* Removed _max_value method and added a dictionary

Related to #5502

* Addressed failing tests and restored _max_value method

* Added xfailing test to switch quicker

* Switch to if/else impl

Reviewed By: vmoens

Differential Revision: D34878986

fbshipit-source-id: 2e8268eda1bff6f5375fc1b1c946a68af539689b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants