-
-
Notifications
You must be signed in to change notification settings - Fork 611
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
Adapt to changes in NNlib to use mode
externally
#308
Conversation
I think the API needs some thought here, and I'd rather not 0/1 (which are unclear to read). Maybe a kwarg like I'm almost tempted to just have a separate |
@MikeInnes I've added |
It seems like convolution mode should be the default with cross correlation as the option. |
I believe we already do; |
@philtomson The nomenclature is actually a bit confusing. |
Yeah, I don't like plain strings for these kinds of things (It's all the rage in Python-land I know, but I think we can do better). It would be good if it was some kind of enum type, I think you can do this with @enum:
EDIT: actually, thinking about it a bit more, I think I like the idea of a separate CrossCorrelation layer/function for this. It would just be a wrapper around Conv with the kernel flipped and passed into Conv. It just makes it cleaner all around to make it explicit, I think, and no special-casing needed in the the actual convolution function which will get used more often anyway. |
Are we doing #423 instead? |
Yes, there was a plan to add an option to |
This works on the changes made in FluxML/NNlib.jl#53 .
(To use mode directly from Flux).