Skip to content

[quant] fill_ path for quantized tensors #43303

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.

Sign up for GitHub

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

Jump to bottom
Closed
z-a-f wants to merge 9 commits into from
Closed

[quant] fill_ path for quantized tensors #43303

z-a-f wants to merge 9 commits into from

Conversation

@z-a-f
Copy link

@z-a-f z-a-f commented Aug 19, 2020 •
edited
Loading

Stack from ghstack:

Differential Revision: D23231947

@z-a-f z-a-f mentioned this pull request Aug 19, 2020
Closed
@z-a-f z-a-f requested review from ailzhang and jerryzh168 August 19, 2020 23:41
@dr-ci
Copy link

dr-ci bot commented Aug 19, 2020 •
edited
Loading

💊 CI failures summary and remediations

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

  • 1/1 failures possibly* introduced in this PR
    • 1/1 non-CircleCI failure(s)

ci.pytorch.org: 1 failed

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 25 times.

ailzhang
ailzhang reviewed Aug 21, 2020
View reviewed changes
Copy link
Contributor

@ailzhang ailzhang left a comment

Choose a reason for hiding this comment

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

ehhh what's the main goal of this PR (perf?) could you please add a bit context?

@z-a-f
Copy link
Author

z-a-f commented Aug 21, 2020

ehhh what's the main goal of this PR (perf?) could you please add a bit context?

It's introducing the fill_ for quantized tensors, which is needed for different ops (specifically for the ConstantPad)

@z-a-f z-a-f requested a review from ailzhang August 21, 2020 04:34
jerryzh168
jerryzh168 reviewed Aug 21, 2020
View reviewed changes
jerryzh168
jerryzh168 reviewed Aug 21, 2020
View reviewed changes
@z-a-f z-a-f requested a review from jerryzh168 August 24, 2020 23:09
raghuramank100
raghuramank100 reviewed Aug 26, 2020
View reviewed changes
raghuramank100
raghuramank100 reviewed Aug 26, 2020
View reviewed changes
raghuramank100
raghuramank100 reviewed Aug 26, 2020
View reviewed changes
@z-a-f z-a-f requested a review from raghuramank100 August 26, 2020 20:11
vkuzo
vkuzo reviewed Aug 28, 2020
View reviewed changes
aten/src/ATen/native/Fill.cpp
Tensor& fill_out(Tensor& self, Scalar value) {
if (self.is_quantized()) {
at::Tensor out = at::ones(self.sizes()).to(kFloat) * value;
out = out.to(self.device());
Copy link
Contributor

@vkuzo vkuzo Aug 28, 2020

Choose a reason for hiding this comment

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

is this needed? Can we assert that out.device() == self.device() instead?

Copy link
Author

@z-a-f z-a-f Aug 28, 2020

Choose a reason for hiding this comment

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

We cannot assert it as the out created in this block. If we don't assign the device, we will have to trust the copy_ to move the tensors across the devices, which is an antipattern :/

@z-a-f z-a-f requested a review from vkuzo August 28, 2020 18:20
@codecov
Copy link

codecov bot commented Aug 28, 2020 •
edited
Loading

Codecov Report

Merging #43303 into gh/z-a-f/53/base will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@                Coverage Diff                @@
##           gh/z-a-f/53/base   #43303   +/-   ##
=================================================
  Coverage             69.24%   69.24%           
=================================================
  Files                   381      381           
  Lines                 47573    47573           
=================================================
  Hits                  32943    32943           
  Misses                14630    14630

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 cce5982...f48d10b. Read the comment docs.

@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Sep 9, 2020

@z-a-f merged this pull request in 1d01fcd.

@facebook-github-bot facebook-github-bot deleted the gh/z-a-f/53/head branch September 12, 2020 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ailzhang ailzhang Awaiting requested review from ailzhang

@jerryzh168 jerryzh168 Awaiting requested review from jerryzh168

@vkuzo vkuzo Awaiting requested review from vkuzo

1 more reviewer

@raghuramank100 raghuramank100 raghuramank100 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Merged

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@z-a-f @facebook-github-bot @vkuzo @jerryzh168 @ailzhang @raghuramank100 @mruberry