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

make tests for empty inputs check zero parameter grads #32820

Closed
wants to merge 3 commits into from

Conversation

ngimel
Copy link
Collaborator

@ngimel ngimel commented Jan 30, 2020

Make batch norm with empty inputs return zero parameter gradients. Now batch norm, group norm and convolutions now return zero grads for parameters, so make tests check that. Fixes some bullet points in #12013 (interpolate is not fixed by this PR, is being fixed in other PRs)

@ngimel ngimel requested a review from albanD January 30, 2020 06:10
@kostmo
Copy link
Member

kostmo commented Jan 30, 2020

💊 CircleCI build failures summary and remediations

As of commit bec1e35:

None of the build failures appear to be your fault.

  • 2/2 broken upstream at merge base 821b6aa since Jan 30

    Please rebase on the viable/strict branch (expand for instructions)

    If your commit is newer than viable/strict, you can try basing on an older, stable commit:

    git fetch origin viable/strict
    git rebase --onto viable/strict $(git merge-base origin/master HEAD)
    

    If your commit is older than viable/strict:

    git fetch origin viable/strict
    git rebase viable/strict
    

    Check out the recency history of this "viable master" tracking branch.

Detailed failure analysis

One may explore the probable reasons each build failed interactively on the Dr. CI website.

🚧 2 upstream failures recognized by patterns:

These builds matched patterns, but were probably caused by upstream breakages:


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.

This comment has been revised 5 times.

Copy link
Collaborator

@albanD albanD left a comment

Choose a reason for hiding this comment

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

I think you're missing the test for batchnorm no?
Otherwise looks good.

aten/src/ATen/native/Normalization.cpp Outdated Show resolved Hide resolved
test/test_nn.py Outdated Show resolved Hide resolved
@ngimel
Copy link
Collaborator Author

ngimel commented Jan 30, 2020

I think you're missing the test for batchnorm no?

Tests for batch norm were already present, they just did not check parameter gradients.

Copy link
Collaborator

@albanD albanD left a comment

Choose a reason for hiding this comment

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

Right I was confused because you add a test for convtranspose :)

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.

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

@facebook-github-bot
Copy link
Contributor

@ngimel merged this pull request in 29fabb1.

facebook-github-bot pushed a commit to facebookresearch/detectron2 that referenced this pull request Feb 4, 2020
Summary:
Transition to native pytorch support that's recently added:
pytorch/pytorch#32709 adds support for conv/convtranspose.
pytorch/pytorch#32820 adds support for batchnorm.
pytorch/pytorch#32401 adds support for groupnorm.

Reviewed By: rbgirshick

Differential Revision: D19658831

fbshipit-source-id: 48834996809b28bdaa6ab6930676a0d54a492460
ttumiel pushed a commit to ttumiel/pytorch that referenced this pull request Mar 4, 2020
Summary:
Make batch norm with empty inputs return zero parameter gradients. Now batch norm, group norm and convolutions now return zero grads for parameters, so make tests check that. Fixes some bullet points in pytorch#12013 (interpolate is not fixed by this PR, is being fixed in other PRs)
Pull Request resolved: pytorch#32820

Differential Revision: D19651470

Pulled By: ngimel

fbshipit-source-id: 96fdd085f9b0e98e91217dd2ac1f30f9c482b8be
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants