Skip to content

Convert assert -> cast. #57458

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
wants to merge 3 commits into from
Closed

Convert assert -> cast. #57458

wants to merge 3 commits into from

Conversation

hameerabbasi
Copy link
Collaborator

Fixes #55868.

@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented May 3, 2021 •
edited
Loading

💊 CI failures summary and remediations

As of commit 697fc27 (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 to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

@facebook-github-bot facebook-github-bot added the oncall: distributed Add this issue/PR to distributed oncall triage queue label May 3, 2021
mrzzd
mrzzd reviewed May 3, 2021
View reviewed changes
KatiaSN602
KatiaSN602 reviewed May 5, 2021
View reviewed changes
@hameerabbasi hameerabbasi marked this pull request as draft May 5, 2021 08:30
@hameerabbasi hameerabbasi force-pushed the fix-assert-cast branch 3 times, most recently from 23a8f1d to 9592ebe Compare May 10, 2021 10:13
@hameerabbasi hameerabbasi marked this pull request as ready for review May 10, 2021 11:44
@codecov
Copy link

codecov bot commented May 10, 2021

Codecov Report

Merging #57458 (697fc27) into master (b84a28b) will decrease coverage by 0.00%.
The diff coverage is 62.50%.

@@            Coverage Diff             @@
##           master   #57458      +/-   ##
==========================================
- Coverage   76.83%   76.82%   -0.01%     
==========================================
  Files        1986     1986              
  Lines      197402   197393       -9     
==========================================
- Hits       151671   151654      -17     
- Misses      45731    45739       +8

rgommers
rgommers approved these changes May 11, 2021
View reviewed changes
Copy link
Collaborator

@rgommers rgommers left a comment

Choose a reason for hiding this comment

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

This LGTM, thanks @hameerabbasi. Removal of all these asserts is quite nice.

LSTM and GRU` are not currently broken with tensor-like's, but adding them to this regression test would be good. If needed can be done afterwards though; this PR should go in for 1.9.0

@rgommers rgommers requested review from walterddr and mruberry May 11, 2021 20:36
@rgommers
Copy link
Collaborator

@walterddr or @mruberry could we land this before the branch cut? It fixes a fairly annoying regression.

walterddr
walterddr approved these changes May 11, 2021
View reviewed changes
Copy link
Contributor

@walterddr walterddr left a comment

Choose a reason for hiding this comment

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

looks good to me.

torch/nn/modules/loss.py
Comment on lines +709 to +710
self.weight: Optional[Tensor]
self.pos_weight: Optional[Tensor]
Copy link
Contributor

Choose a reason for hiding this comment

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

should these be in the class scope? (https://www.python.org/dev/peps/pep-0526/#id9)

@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

@walterddr merged this pull request in 46e4b2d.

krshrimali pushed a commit to krshrimali/pytorch that referenced this pull request May 19, 2021
@hameerabbasi
Convert assert -> cast. (pytorch#57458)
03427fe 
Summary:
Fixes pytorch#55868.

Pull Request resolved: pytorch#57458

Reviewed By: mruberry

Differential Revision: D28365745

Pulled By: walterddr

fbshipit-source-id: 35cc3fa85f87b0ef98cf970f620ab909d240c7be
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rgommers rgommers rgommers approved these changes

@albanD albanD Awaiting requested review from albanD

@H-Huang H-Huang Awaiting requested review from H-Huang

@jbschlosser jbschlosser Awaiting requested review from jbschlosser

@mingzhe09088 mingzhe09088 Awaiting requested review from mingzhe09088

@mrshenli mrshenli Awaiting requested review from mrshenli

@pritamdamania87 pritamdamania87 Awaiting requested review from pritamdamania87

@rohan-varma rohan-varma Awaiting requested review from rohan-varma

@wayi1 wayi1 Awaiting requested review from wayi1

@zhaojuanmao zhaojuanmao Awaiting requested review from zhaojuanmao

@guilhermeleobas guilhermeleobas Awaiting requested review from guilhermeleobas

@mruberry mruberry Awaiting requested review from mruberry

+3 more reviewers

@mrzzd mrzzd mrzzd left review comments

@KatiaSN602 KatiaSN602 KatiaSN602 left review comments

@walterddr walterddr walterddr approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
cla signed Merged oncall: distributed Add this issue/PR to distributed oncall triage queue open source
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Using __torch_function__ with RNN module
7 participants
@hameerabbasi @facebook-github-bot @rgommers @walterddr @mrzzd @KatiaSN602 @pytorchbot