Conv Shift Operator #4591
Conv Shift Operator #4591
Conversation
|
I think this code does not work for GPU - I will try to fix that. For an overall review, it may be better to wait until that is done. (That said, this code will probably stay about the same for CPU, so I would appreciate feedback on that any time.) |
paddle/operators/conv_shift_op.h
Outdated
| namespace paddle { | ||
| namespace operators { | ||
|
|
||
| using Tensor = framework::Tensor; |
There was a problem hiding this comment.
use using in the header file violate the google style, see the detail in Namespace
Do not use Namespace aliases at namespace scope in header files except in explicitly marked internal-only namespaces, because anything imported into a namespace in a header file becomes part of the public API exported by that file.
By the way, I think
using framework::Tensor;is enough here.
Some old operators have a wrong guide, sorry for that.
There was a problem hiding this comment.
I just see this comment by chance.
The Google style says:
Do not use Namespace aliases at namespace scope in header files
I think:
using Tensor = framework::Tensorthis usage does not violate the above Google style because it is just a type alias, not a namespace alias. But, I admit that using type alias in such a large scope is not a good thing, so you are right...
paddle/operators/conv_shift_op.h
Outdated
| size_t y_width = y_dims[1]; | ||
| size_t y_half_width = (y_width - 1) / 2; | ||
|
|
||
| // The below trades code duplication for efficiency (keeping the if |
There was a problem hiding this comment.
good point!
An if condition inside a three-fold for loop will cost a lot of time.
Limitations: - both gradient outputs must be specified and are always computed - explicit for loops => could be optimized in various ways (e.g., different memory layout)
fix case when not all output gradients desired
|
Thank you for the feedback! I moved the |
|
This PR LGTM, quite clean and well commented~ |
This closes #4574 .
This is an unoptimized Eigen-based port of the existing code:
Examples of possible future optimizations include:
out.col(i) = x.col(index) * y.col(j). This would make the most sense if we either had column-major storage order or if the batch dimension were last rather than first.I would probably leave such optimizations to the future when we can profile an actual use case and see whether the improvements are worth it.
@dzhwinter Would you mind taking a look? This is my first PR to this repo - your feedback would be much appreciated!