Skip to content

Mypy fixes for pytorch master #52090

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 6 commits into from
Closed

Mypy fixes for pytorch master #52090

wants to merge 6 commits into from

Conversation

guilhermeleobas
Copy link
Collaborator

@guilhermeleobas guilhermeleobas commented Feb 10, 2021 •
edited
Loading

This PR adds fixes mypy issues on the current pytorch main branch. In special, it replaces occurrences of np.bool/np.float to np.bool_/np.float64, respectively:

test/test_numpy_interop.py:145: error: Module has no attribute "bool"; maybe "bool_" or "bool8"?  [attr-defined]
test/test_numpy_interop.py:159: error: Module has no attribute "float"; maybe "float_", "cfloat", or "float64"?  [attr-defined]

@guilhermeleobas guilhermeleobas added the module: typing Related to mypy type annotations label Feb 10, 2021
@guilhermeleobas guilhermeleobas self-assigned this Feb 10, 2021
@guilhermeleobas guilhermeleobas marked this pull request as ready for review February 10, 2021 23:58
@codecov
Copy link

codecov bot commented Feb 11, 2021 •
edited
Loading

Codecov Report

Merging #52090 (2a5f573) into master (2900cf2) will increase coverage by 0.16%.
The diff coverage is 75.00%.

@@            Coverage Diff             @@
##           master   #52090      +/-   ##
==========================================
+ Coverage   80.57%   80.74%   +0.16%     
==========================================
  Files        1957     1955       -2     
  Lines      214523   214413     -110     
==========================================
+ Hits       172852   173127     +275     
+ Misses      41671    41286     -385

@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Feb 12, 2021 •
edited
Loading

💊 CI failures summary and remediations

As of commit de159d6 (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.

rgommers
rgommers requested changes Feb 12, 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.

Thanks @guilhermeleobas. The bool and float changes were already fixed in another PR a couple of days ago, so you can drop those changes it looks like.

rgommers
rgommers approved these changes Feb 16, 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.

LGTM now, thanks @guilhermeleobas.

CI failures are unrelated.

@rgommers rgommers requested review from malfet and walterddr February 16, 2021 19:42
malfet
malfet requested changes Feb 16, 2021
View reviewed changes
Copy link
Contributor

@malfet malfet left a comment

Choose a reason for hiding this comment

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

Can you please add some explanation to the PR description, mentioning why np.bool needs to be changed to np.bool_ and np.float to np.float64

And, as @rgommers mentioned, please use np.float64 rather than np.float_

facebook-github-bot
facebook-github-bot reviewed Feb 16, 2021
View reviewed changes
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.

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

rgommers
rgommers reviewed Feb 16, 2021
View reviewed changes
@ejguan ejguan added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Feb 17, 2021
facebook-github-bot
facebook-github-bot reviewed Feb 17, 2021
View reviewed changes
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.

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

@facebook-github-bot
Copy link
Contributor

@malfet merged this pull request in 975d9f2.

xsacha pushed a commit to xsacha/pytorch that referenced this pull request Mar 31, 2021
@guilhermeleobas
Mypy fixes for pytorch master (pytorch#52090)
cb4762a 
Summary:
This PR adds fixes mypy issues on the current pytorch main branch. In special, it replaces occurrences of `np.bool/np.float` to `np.bool_/np.float64`, respectively:

```
test/test_numpy_interop.py:145: error: Module has no attribute "bool"; maybe "bool_" or "bool8"?  [attr-defined]
test/test_numpy_interop.py:159: error: Module has no attribute "float"; maybe "float_", "cfloat", or "float64"?  [attr-defined]
```

Pull Request resolved: pytorch#52090

Reviewed By: walterddr

Differential Revision: D26469596

Pulled By: malfet

fbshipit-source-id: e55a5c6da7b252469e05942e0d2588e7f92b88bf
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

@facebook-github-bot facebook-github-bot facebook-github-bot left review comments

@malfet malfet malfet requested changes

+1 more reviewer

@walterddr walterddr walterddr approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@guilhermeleobas guilhermeleobas

Labels

cla signed Merged module: typing Related to mypy type annotations open source triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@guilhermeleobas @facebook-github-bot @rgommers @malfet @walterddr @pytorchbot @ejguan