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 Parallel layer #1462

Merged
merged 15 commits into from
Jan 14, 2021
Merged

Add Parallel layer #1462

merged 15 commits into from
Jan 14, 2021

Conversation

darsnack
Copy link
Member

@darsnack darsnack commented Jan 12, 2021

Since #1289 stalled, I have added an implementation of Parallel with some of the changes we discussed during ML calls. This version excludes most of the structural layers in #1289 like Join, Split, and Nop. I also added the ability for the user to specify the reduction operator. If it is acceptable, I would like to remap SkipConnection to Parallel (not a deprecation exactly).

The reason for submitting this PR now is because I am creating pre-trained weights for the networks in FluxML/Metalhead.jl#70, and there is a lot of code that can be replaced with a Parallel. So, I'd like to have Parallel in Flux before continuing with training to make the process easier.

PR Checklist

  • Tests are added
  • Entry in NEWS.md
  • Documentation, if applicable
  • Final review from @DhairyaLGandhi (for API changes).

cc @CarloLucibello

@darsnack
Copy link
Member Author

Also, I'd like to add that outputsize worked out of the box with no changes when I added Parallel. So that's a small win to celebrate.

src/layers/basic.jl Outdated Show resolved Hide resolved
Copy link
Member

@DhairyaLGandhi DhairyaLGandhi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, do we also want to add the functionality from having multiple arguments in the forward pass as in parallel execution and reduction? Seems like there's some additional functionality in #1289

src/layers/basic.jl Outdated Show resolved Hide resolved
src/layers/basic.jl Outdated Show resolved Hide resolved
src/layers/basic.jl Outdated Show resolved Hide resolved
@darsnack
Copy link
Member Author

Adding multiple input arguments would be like Join from #1289. We could add a Vararg version of Parallel that implements that behavior.

@darsnack
Copy link
Member Author

Actually on second thought, maybe we shouldn't have the multi-arg version? It complicates the semantics on Chain, and I don't think it is used in practice for anything.

@darsnack
Copy link
Member Author

@DhairyaLGandhi any decision on keeping vs excluding multiple input arguments for the layer? I would recommend excluding.

src/layers/basic.jl Outdated Show resolved Hide resolved
src/layers/basic.jl Outdated Show resolved Hide resolved
Copy link
Member

@DhairyaLGandhi DhairyaLGandhi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The vararg AbstractArray method is the one I had in mind. I would also want to see some addition to the docs that talks about implementing the Join and Split because I feel we would inevitably have questions about wanting them.

Otherwise, lgtm

src/layers/basic.jl Outdated Show resolved Hide resolved
src/layers/basic.jl Outdated Show resolved Hide resolved
@darsnack
Copy link
Member Author

What about SkipConnection being syntactic sugar for Parallel with identity on one of the paths?

@DhairyaLGandhi
Copy link
Member

I'm in favour of retaining the struct because it's a very common case, but rewriting it in a follow up

@DhairyaLGandhi
Copy link
Member

DhairyaLGandhi commented Jan 13, 2021

@darsnack I was hoping the docs would show how to implement Join in terms of Parallel.

@darsnack
Copy link
Member Author

Note that Join is the same as Parallel with variable arguments. There isn't much work required to implement Join in terms of Parallel. So I just added a subsection to the docs. This should be more instructive for users.

@darsnack
Copy link
Member Author

@DhairyaLGandhi bump

Copy link
Member

@DhairyaLGandhi DhairyaLGandhi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm, thanks @darsnack

@DhairyaLGandhi
Copy link
Member

bors r+

@darsnack
Copy link
Member Author

@bhvieira Currently, it is still in Flux as it was before. My plan was not to deprecate it but to define:

SkipConnection(layers, connection) = Parallel(connection, layers, identity)

I think the SkipConnection syntax/nomenclature is still useful and meaningful.

@bhvieira
Copy link
Contributor

@darsnack Yeah, I deleted my comment before seeing yours actually, that's what I had in mind! Good job btw!

@bors
Copy link
Contributor

bors bot commented Jan 14, 2021

Build succeeded:

@bors bors bot merged commit 33f99ef into FluxML:master Jan 14, 2021
@darsnack darsnack deleted the darsnack/parallel-layer branch January 14, 2021 16:48
@ToucheSir ToucheSir mentioned this pull request Feb 11, 2021
@DhairyaLGandhi
Copy link
Member

DhairyaLGandhi commented Feb 11, 2021

Can we add follow on testing on the GPU, that seems to have been missed? My bad. @darsnack

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.

5 participants