Unify *PoolLayer interfaces, new default ignore_border=True

[f0k]

Resolves #371 (users would get slow pooling layers by default because ignore_border=False prevents cuDNN from being used). Adds a note about ignore_border=True slowing down computations. Unifies *PoolLayer interfaces to follow the same order of pool_size, stride, pad, ignore_border, (mode), they were inconsistent before and sometimes not even matching the order in their own docstrings. Adapts the tests.

[skaae]

Does the problem only occur if you use the non DNN MaxPool layers? So if I use a MaxPool2DDNNLayer I would get an error if dnn cannot be used?

Couldn't we raise i warning if the maxpool layer fallback on CPU? It is pretty subtle but the performance hit you take seems to be very large?

[f0k]

So if I use a MaxPool2DDNNLayer I would get an error if dnn cannot be used?

If you use a MaxPool2DDNNLayer, you won't be using it with ignore_border=False by definition (it didn't have an ignore_border argument before, and it does not support including partial pooling regions). And yes, that one will always use cuDNN or not work at all.

The problem only occurs if you're using the ordinary MaxPool2DLayer, and the performance regression comes totally unexpected (and probably even unnoticed), just because of a setting that most users will not even have thought of looking up the documentation for.

Couldn't we raise i warning if the maxpool layer fallback on CPU? It is pretty subtle but the performance hit you take seems to be very large?

We could raise a warning when combining ignore_border=False with pool_size != stride, that's when the CPU version will be used in the current version of Theano. However, there's also a performance hit when combining ignore_border=False with pool_size == stride, because it will use Theano's max-pooling implementation instead of cuDNN's. Shall we raise a warning whenever somebody uses ignore_border=False, but still have ignore_border=False be the default? That would be a bit silly.

I think we should use the fact that Lasagne hasn't been released yet and just change it, for the benefit of future users. It's sad we hadn't noticed this like half a year ago, but it's good we noticed before the release.

[skaae]

Thanks for the clarification. I'm definitely in favor of changing the default to True. I think its not a problem chaning this if we mention the change in the release notes.

[f0k]

I'm definitely in favor of changing the default to True.

Cool! Any other opinions? There's only a few hours left if we're releasing today!

I think its not a problem chaning this if we mention the change in the release notes.

We don't really have release notes we can mention this in, we only announce breaking changes on the mailing list. For future releases, we could highlight such things in the changelog, but we're going to avoid breaking changes after the release anyway.

[craffel]

I'm definitely in favor of changing the default to True.

Cool! Any other opinions? There's only a few hours left if we're releasing today!

I also think it's worth changing the default pre-release, with an announcement. Releases are when users should expect changes in behavior, and this one is worth it.

[benanne]

Agree, let's just change it.