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

Add SeparableConv1D layer #125

Open
6 tasks
zaleslaw opened this issue Jun 15, 2021 · 4 comments
Open
6 tasks

Add SeparableConv1D layer #125

zaleslaw opened this issue Jun 15, 2021 · 4 comments
Assignees

Comments

@zaleslaw
Copy link
Collaborator

zaleslaw commented Jun 15, 2021

Currently, the support for SeparableConv1D is missed, and it would be great to add support for this layer. The desired PR addressing this issue should include:

  • Implementation of layer class named as SeparableConv1D (you can take inspiration from the implementation of Conv1D and SeparableConv2D as reference)
  • Common hierarchy of all SeparableConv layers with abstract class with the common functionality
  • Documentation of layer and all non-private methods
  • JUnit tests in api module
  • Support for export of layer to JSON (see ModelSaver.kt)
  • Support for import of layer from JSON (see ModelLoader.kt)

Also, if needed, you can take a look at Keras documentation for SeparableConv1D .

NOTE: for the moment, there is no need to add support for "data format" (i.e., channels last vs. channels first) in your PR; you can assume the channels are always in the last dimension.

P.S. If you want to take this ticket, please leave the comment below
P.P.S Read the Contributing Guidelines.

@zaleslaw zaleslaw added the good first issue Good for newcomers label Jun 15, 2021
@zaleslaw zaleslaw added this to the 0.3 milestone Jun 15, 2021
@dosier
Copy link
Contributor

dosier commented Jun 18, 2021

I might be able to do this, not quite sure if I understand it well enough to do this properly.

Is SerpableConv supposed to override AbstractConv or stand on its own? I take it should override. I noted in the Keras code that SerpableConv has support for two kernels (depthwise_kernel and pointwise_kernel), so should the parent kernel in AbstractConv be ignored here?

@zaleslaw
Copy link
Collaborator Author

zaleslaw commented Jun 21, 2021

I suggest creating AbstractSeparableConv looking to the AbstractConv, but without inheritance.
There are a lot of different fields there: initializers, regularizers, kernels.

If it makes sense, we merge these class hierarchies.

I'll assign this ticket to you; please notify me if you will not do this.

@dosier
Copy link
Contributor

dosier commented Jun 24, 2021

I will see if I can get this one done in the weekend.

dosier added a commit to dosier/KotlinDL that referenced this issue Jul 8, 2021
- still have to write tests, and do the 1D variant
- this also fixed a bug in WeightLoader that did not load the bias weights correctly for SeparableConv2D.kt
@zaleslaw
Copy link
Collaborator Author

Hi, @dosier Are you interested in continuing working on this issue?

@zaleslaw zaleslaw modified the milestones: 0.3, 0.4 Sep 13, 2021
@zaleslaw zaleslaw assigned juliabeliaeva and unassigned dosier Oct 12, 2021
@zaleslaw zaleslaw removed the good first issue Good for newcomers label Dec 15, 2021
@zaleslaw zaleslaw modified the milestones: 0.4, 0.5 Feb 17, 2022
@ermolenkodev ermolenkodev removed this from the 0.5 milestone Sep 21, 2022
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

No branches or pull requests

4 participants