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

Support for non-square kernels. Added kernel_x and kernel_y variable to ... #76

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rmanor
Copy link

@rmanor rmanor commented Dec 4, 2013

...convolution layer with backward compatibility for kernelsize.

…to convolution layer with backward compatibility for kernelsize.
@rasmusbergpalm
Copy link
Owner

Looks good. Some points:
make kernelsize a vector, don't be backwards compatible, fix examples to use new style and add assertions in cnnsetup to verify correct kernelsize (i.e. positive integer vector) in order to fail fast.
Add a test that using non-square and square kernels works as intended. (i.e. the outputmaps are the expected size, for a known input the computed output is correct, etc.)

@rasmusbergpalm rasmusbergpalm mentioned this pull request Dec 4, 2013
@albertoandreottiATgmail
Copy link
Contributor

Hi rmanor, rasmus,

this is no exactly what my pull request was about, I wanted to have non squared inputs(e.g., an spectrogram of 50x600). I'll work in a test case for checking gradients when non squared inputs are present.
I have a test case on language detection that uses non squared inputs, but it requires huge data(arround 3GBs) and it takes many hours(~15) to complete. I'll try to create a toy test case with artificial inputs. So rmanor, please do not work in the same thing, focus on the non squared kernel.
Contact me if you want to talk, I'll be glad.

@rmanor
Copy link
Author

rmanor commented Dec 4, 2013

Hi,

My change is about non squared kernels, not inputs. As far as I can tell
there is currently no problem with non squared inputs, I already used it
with non squared inputs.

  • Ran

On Wed, Dec 4, 2013 at 3:05 PM, albertoandreottiATgmail <
notifications@github.com> wrote:

Hi rmanor, rasmus,

this is no exactly what my pull request was about, I wanted to have non
squared inputs(e.g., an spectrogram of 50x600). I'll work in a test case
for checking gradients when non squared inputs are present.
I have a test case on language detection that uses non squared inputs, but
it requires huge data(arround 3GBs) and it takes many hours(~15) to
complete. I'll try to create a toy test case with artificial inputs. So
rmanor, please do not work in the same thing, focus on the non squared
kernel.
Contact me if you want to talk, I'll be glad.


Reply to this email directly or view it on GitHubhttps://github.com//pull/76#issuecomment-29802786
.

@albertoandreottiATgmail
Copy link
Contributor

Hey Ran, are you talking about the CNN? are you sure it worked fine? Would you mind taking a look at my pull request to see if my changes make sense to you? Alberto.

@rmanor
Copy link
Author

rmanor commented Dec 4, 2013

Yes, CNNs. Pretty sure, I will re-check.
Why? Where did you see problems?

  • Ran

On Wed, Dec 4, 2013 at 3:17 PM, albertoandreottiATgmail <
notifications@github.com> wrote:

Hey Ran, are you talking about the CNN? are you sure it worked fine?


Reply to this email directly or view it on GitHubhttps://github.com//pull/76#issuecomment-29803498
.

@albertoandreottiATgmail
Copy link
Contributor

Hey!, sorry for the confusion. You're right, the net would work as it is with non squared inputs. I needed this non squared average operation for reducing vectors(say 50x1) to a single variable, the only way to achieve this was with an average operation using a non squared kernel.
So, just to give some context, I wanted to convert each map to a variable, near the output layer, that was achieved by this change. I think you can introduce your changes withouth conflicting with mine.
Ramus: is it ok to add separate test cases for each of these changes?

@rasmusbergpalm
Copy link
Owner

Yes! These should definitely be implemented and tested separately.

@taygunk
Copy link

taygunk commented May 5, 2014

anyone has a progress on this issue?

@albertoandreottiATgmail
Copy link
Contributor

I'll take a look during the weekend, if you could wait. Thanks, Alberto.

On 5 May 2014 17:26, Taygun Kekec notifications@github.com wrote:

anyone progress on this issue?


Reply to this email directly or view it on GitHubhttps://github.com//pull/76#issuecomment-42234915
.

José Pablo Alberto Andreotti.
Tel: 54 351 4730292
Móvil: 54351156526363.
MSN: albertoandreotti@gmail.com
Skype: andreottialberto

@albertoandreottiATgmail
Copy link
Contributor

I remember we had problems with the convn() function in Octave. My branch had a fix for this. Now Octave 3.8.1 is released, so I'll test everything under the new version and see what happens.

@rasmusbergpalm
Copy link
Owner

What's the status on this one?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants