Skip to content

Conversation

@ejoebstl
Copy link
Contributor

@ejoebstl ejoebstl commented Apr 19, 2018

The gradient of LP-Pooling is undefined when the sum of all inputs to the power of p is zero. The current implementation will yield NAN in this case.

I've added a ReLU unit to set the gradient to zero if the sum of all input elements to the power of p is zero.

This would fix issue #6765.

@ezyang
Copy link
Contributor

ezyang commented Apr 22, 2018

@pytorchbot retest this please

@ejoebstl
Copy link
Contributor Author

Before you merge, please make sure that this is really desired. The hack works well in practice (at least better than having NANs), but I'm not sure if other values (-1, +1) would be better.

@ezyang
Copy link
Contributor

ezyang commented Apr 23, 2018

CC @soumith

@fmassa
Copy link
Member

fmassa commented Apr 24, 2018

I think it would be good to add a test fornthis case as well.
Another option would be to add an eps before taking the pow. This would make the gradients finite, but maybe they could be very large. But the resulting expression would be much simpler

@ejoebstl
Copy link
Contributor Author

@fmassa Lets move the discussion to the issue #6765.

@soumith soumith merged commit 645ad7a into pytorch:master Apr 26, 2018
@soumith
Copy link
Contributor

soumith commented Apr 26, 2018

i think this is reasonable. I've merged it.

Jorghi12 pushed a commit to wsttiger/pytorch that referenced this pull request May 10, 2018
* Added ReLU unit to LP pooling, so the gradient does not become NAN if all inputs are zero.

* Added workaround for odd p. Added a bit of doc.

* Make the linter happy.
weiyangfb pushed a commit to weiyangfb/pytorch that referenced this pull request Jun 11, 2018
* Added ReLU unit to LP pooling, so the gradient does not become NAN if all inputs are zero.

* Added workaround for odd p. Added a bit of doc.

* Make the linter happy.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants