Skip to content
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

[ONNX] Preprocess index_put with bool inputs to masked_scatter/masked_fill #45584

Conversation

shubhambhokare1
Copy link
Collaborator

@shubhambhokare1 shubhambhokare1 commented Sep 30, 2020

When the input to an indexing operation is a boolean, for example array[True] = value,
the subsequent index_put node formed needs to be converted to masked_scatter/masked_fill node based on the type of val the indexing node is equated. If that value is just a single scalar, then we use the masked_fill functionality and if value is a tensor of appropriate size, we use the masked_scatter functionality.

Fixes #34054

@facebook-github-bot facebook-github-bot added the oncall: jit Add this issue/PR to JIT oncall triage queue label Sep 30, 2020
@dr-ci
Copy link

dr-ci bot commented Sep 30, 2020

💊 CI failures summary and remediations

As of commit 1367144 (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).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 6 times.

@KsenijaS
Copy link
Contributor

Can you add a better description, also try to explain why and when you use aten::masked_scatter and aten::masked_fill

@shubhambhokare1 shubhambhokare1 changed the title [ONNX] Convert index_put with bool inputs to masked_scatter [ONNX] Convert index_put with bool inputs to masked_scatter/masked_fill Sep 30, 2020
@shubhambhokare1 shubhambhokare1 changed the title [ONNX] Convert index_put with bool inputs to masked_scatter/masked_fill [ONNX] Preprocess index_put with bool inputs to masked_scatter/masked_fill Sep 30, 2020
@codecov
Copy link

codecov bot commented Oct 1, 2020

Codecov Report

Merging #45584 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #45584   +/-   ##
=======================================
  Coverage   68.50%   68.50%           
=======================================
  Files         408      408           
  Lines       52484    52484           
=======================================
  Hits        35952    35952           
  Misses      16532    16532           

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 77cd8e0...1367144. Read the comment docs.

@Enolerobotti
Copy link

Thank you @shubhambhokare1! This patch actually resolves #34054. @apaszke I would recommend this patch

Copy link
Collaborator

@BowenBao BowenBao 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!

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

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

@shubhambhokare1
Copy link
Collaborator Author

@bzinodev Any changes required on my end for the PR to be merged?

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

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

@facebook-github-bot
Copy link
Contributor

@bzinodev merged this pull request in 9d389b1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Merged oncall: jit Add this issue/PR to JIT oncall triage queue open source
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Export to ONNX models including logical masks
7 participants