Expose is_signed for dtype #29511

vishwakftw · 2019-11-09T22:54:41Z

Changelog:

Expose is_signed for torch.dtype by modifying torch/csrc/Dtype.cpp
Allow half, bfloat16 and bool to also been "known" by the isSignedType function

Test Plan:

Add tests in test/test_torch.py

Changelog: - Expose is_signed for torch.dtype by modifying torch/csrc/Dtype.cpp - Allow bfloat16 and bool to also been "known" by the isSignedType function Test Plan: - Add tests in test/test_torch.py

c10/core/ScalarType.h

…se-is_signed

albanD

Thanks making the changes!

facebook-github-bot

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

nairbv

Thanks!

vishwakftw · 2019-11-12T22:04:51Z

@albanD is this good to be merged?

albanD · 2019-11-13T00:54:29Z

@vishwakftw Sorry I was checking with the people working on BFloat16 to make sure this special case was useful.
Given this I think you don't need this special case, and you can add it back to the main macro :)

vishwakftw · 2019-11-13T00:58:15Z

Sure, thanks @albanD.

Just a side note, I think you might have tagged the wrong person.

albanD · 2019-11-13T01:14:51Z

Yes, my bad, fixed that.
This will be good to merge once you do the change.

vishwakftw · 2019-11-13T19:37:16Z

@albanD I've fixed it. Thanks for the pointer about BFloat16.

facebook-github-bot

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

vishwakftw · 2019-11-14T17:09:46Z

@albanD is this good to merge? I see some failing internal tests.

albanD · 2019-11-14T21:41:54Z

Yes, I was waiting for all the tests to run, failures are unrelated.

facebook-github-bot · 2019-11-15T20:48:27Z

@albanD merged this pull request in 69e343f.

Summary: Changelog: - Expose is_signed for torch.dtype by modifying torch/csrc/Dtype.cpp - Allow half, bfloat16 and bool to also been "known" by the isSignedType function Pull Request resolved: pytorch#29511 Test Plan: - Add tests in test/test_torch.py Closes pytorch#29475 Differential Revision: D18439030 Pulled By: albanD fbshipit-source-id: 4b1f9da70c1c8dfd0a5bc028b6936acd1c64af47

gchanan · 2019-11-26T18:00:13Z

Umm...are quantized dtypes signed? It's not clear to me that the underlying dtype is relevant here, that's just a representation, i.e. I believe you can have negative values with a quint8 dtype. Is that correct @jerryzh168 ?

jerryzh168 · 2019-11-26T18:33:10Z

Umm...are quantized dtypes signed? It's not clear to me that the underlying dtype is relevant here, that's just a representation, i.e. I believe you can have negative values with a quint8 dtype. Is that correct @jerryzh168 ?

yeah, quint8 dtype can represent negative floating point values, since we have scale and zero_point as well.

jerryzh168 · 2019-11-26T18:34:39Z

We need to know how is_signed is used first, then we can decide what should be is_signed for quantized dtypes

nairbv · 2019-11-26T19:49:04Z

We need to know how is_signed is used first, then we can decide what should be is_signed for quantized dtypes

Should we change this to throw an error for quantized types until we figure it out? The idea is for this to be usable from python, so could be used for anything. Within PyTorch I think so-far it's only used in this unit test:
https://github.com/pytorch/pytorch/pull/29231/files#diff-9996665f82f52030836eb8657057cfadR2645

jerryzh168 · 2019-11-27T18:16:29Z

@nairbv yeah looks like is_signed doesn't make sense for quantized dtype in this case, we should throw an error if people call is_signed for quantized dtypes.

nairbv · 2019-11-27T18:28:30Z

@gchanan how do we feel about having access to a property raise an error? The behavior of torch.qint8.is_signed raising an error seems a bit odd to me where there's no function call, though should be an easy change to make.

nairbv · 2019-11-27T18:56:23Z

though should be an easy change to make.

Actually it seems if you throw an error during a property access it dumps core.

Summary: When we introduced dtype.is_signed we allowed for support of quantized types, but we're not sure what the correct result should be. See discussion at #29511 [ghstack-poisoned]

Summary: When we introduced dtype.is_signed we allowed for support of quantized types, but we're not sure what the correct result should be. See discussion at #29511 ghstack-source-id: fffc818 Pull Request resolved: #30527

nairbv · 2019-11-27T19:10:21Z

Nevermind, got it working:

#30527

Still seems a bit weird to me though to raise an error on an attribute, without a function call.

Summary: When we introduced dtype.is_signed we allowed for support of quantized types, but we're not sure what the correct result should be. See discussion at #29511 [ghstack-poisoned]

Summary: When we introduced dtype.is_signed we allowed for support of quantized types, but we're not sure what the correct result should be. See discussion at #29511 ghstack-source-id: 4ccb037 Pull Request resolved: #30527

Summary: When we introduced dtype.is_signed we allowed for support of quantized types, but we're not sure what the correct result should be. See discussion at #29511 Differential Revision: [D18765410](https://our.internmc.facebook.com/intern/diff/D18765410)

Summary: When we introduced dtype.is_signed we allowed for support of quantized types, but we're not sure what the correct result should be. See discussion at #29511 ghstack-source-id: 3b2d4db Pull Request resolved: #30527

Summary: Pull Request resolved: #30527 When we introduced dtype.is_signed we allowed for support of quantized types, but we're not sure what the correct result should be. See discussion at #29511 Test Plan: Imported from OSS Differential Revision: D18765410 Pulled By: nairbv fbshipit-source-id: c87cfe999b604cfcbbafa561e04d0d5cdbf41e6d

Summary: Pull Request resolved: pytorch#30527 When we introduced dtype.is_signed we allowed for support of quantized types, but we're not sure what the correct result should be. See discussion at pytorch#29511 Test Plan: Imported from OSS Differential Revision: D18765410 Pulled By: nairbv fbshipit-source-id: c87cfe999b604cfcbbafa561e04d0d5cdbf41e6d

Expose is_signed for dtype

06daae1

Changelog: - Expose is_signed for torch.dtype by modifying torch/csrc/Dtype.cpp - Allow bfloat16 and bool to also been "known" by the isSignedType function Test Plan: - Add tests in test/test_torch.py

vishwakftw requested review from albanD and nairbv November 9, 2019 22:54

albanD reviewed Nov 11, 2019

View reviewed changes

c10/core/ScalarType.h Outdated Show resolved Hide resolved

vishwakftw added 3 commits November 11, 2019 14:00

Add quantized types in isSignedType

85abb2c

Merge branch 'master' of https://github.com/pytorch/pytorch into expo…

24be863

…se-is_signed

simplify code

b6eda5f

albanD approved these changes Nov 11, 2019

View reviewed changes

facebook-github-bot reviewed Nov 11, 2019

View reviewed changes

nairbv approved these changes Nov 12, 2019

View reviewed changes

Don't special case BFloat16

edb2a81

facebook-github-bot reviewed Nov 13, 2019

View reviewed changes

facebook-github-bot closed this in 69e343f Nov 15, 2019

facebook-github-bot added the merged label Nov 15, 2019

nairbv mentioned this pull request Nov 27, 2019

Raise an error for is_signed on quantized types #30527

Closed

vishwakftw added the open source label Dec 1, 2019

vishwakftw deleted the expose-is_signed branch December 20, 2019 19:25

mruberry added the Merged label Oct 28, 2020

Expose is_signed for dtype #29511

Expose is_signed for dtype #29511

Uh oh!

Conversation

vishwakftw commented Nov 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

albanD left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

nairbv left a comment

Choose a reason for hiding this comment

Uh oh!

vishwakftw commented Nov 12, 2019

Uh oh!

albanD commented Nov 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vishwakftw commented Nov 13, 2019

Uh oh!

albanD commented Nov 13, 2019

Uh oh!

vishwakftw commented Nov 13, 2019

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

vishwakftw commented Nov 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

albanD commented Nov 14, 2019

Uh oh!

facebook-github-bot commented Nov 15, 2019

Uh oh!

gchanan commented Nov 26, 2019

Uh oh!

jerryzh168 commented Nov 26, 2019

Uh oh!

jerryzh168 commented Nov 26, 2019

Uh oh!

nairbv commented Nov 26, 2019

Uh oh!

jerryzh168 commented Nov 27, 2019

Uh oh!

nairbv commented Nov 27, 2019

Uh oh!

nairbv commented Nov 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nairbv commented Nov 27, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

vishwakftw commented Nov 9, 2019 •

edited

Loading

albanD commented Nov 13, 2019 •

edited

Loading

vishwakftw commented Nov 14, 2019 •

edited

Loading

nairbv commented Nov 27, 2019 •

edited

Loading