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

Workaround for inplace max pooling issue [merged] #3574

Closed
wants to merge 2 commits into from

Conversation

thatguymike
Copy link
Contributor

CuDNN assumes layers are not being modified in place, thus breaking our index tracking for updates in some cases in Caffe. Until there is a workaround in Caffe (index management) or cuDNN, use Caffe
engine for max pooling, or don't use in place layers after max pooling layers

@shelhamer shelhamer added the bug label Jan 20, 2016
@shelhamer
Copy link
Member

This solution forces all max pooling to be done by the Caffe engine without offering an override. Correctness is the best default, so I'm fine with that, but I wanted to check that this global switch is desired.

Note this is a particular instance of #2015 highlighted in #2688.

@thatguymike
Copy link
Contributor Author

For now, I think this is the best solution until we understand the way we want to fundamentally handle in-place more generally. There is a performance difference, but for most folks doing training, shouldn't matter much. The fundamental issue with inplace operations is going to bite us again and again I fear. We can choose to try to address the fundamental issue, but without this change or similar workaround we are breaking people's nets.

@shelhamer
Copy link
Member

@thatguymike agreed. Please adjust the indentation (4 spaces to align with code) for merge -- for some reason lint is not checking the style.

Thanks for the workaround.

@thatguymike
Copy link
Contributor Author

The whole file appears to be 2 space indent for code, which is what I have... Or do you just mean you want the comment block indented? Lint is enforcing style, it made me add the other style changes you prefer like spaces after if's.

shelhamer added a commit that referenced this pull request Jan 20, 2016
  Substitute Caffe engine for cuDNN max pooling
  as workaround for in-place after max pooling issue
@shelhamer
Copy link
Member

Ok, merged in 37066eb (with changes squashed into a single commit). Closing this PR.

@thatguymike It was the comments. I thought our lint check caught out-of-line comments but apparently not.

@shelhamer shelhamer closed this Jan 20, 2016
@shelhamer shelhamer changed the title Workaround for inplace max pooling issue Workaround for inplace max pooling issue [merged] Jan 20, 2016
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.

None yet

2 participants