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

Parallel layer doesn't need to be tied to array input #1673

Closed
rkurchin opened this issue Jul 22, 2021 · 8 comments · Fixed by #1674
Closed

Parallel layer doesn't need to be tied to array input #1673

rkurchin opened this issue Jul 22, 2021 · 8 comments · Fixed by #1674

Comments

@rkurchin
Copy link

(m::Parallel)(xs::Vararg{<:AbstractArray}) = mapreduce((f, x) -> f(x), m.connection, m.layers, xs)

This line is needlessly restrictive, and breaks for a use case I'd like to make involving a custom type in my ChemistryFeaturization and AtomicGraphNets packages where the input to an AGNConv layer can be a FeaturizedAtoms type.

I'm not sure if the solution is just to change AbstractArray to Any or what...

@DhairyaLGandhi
Copy link
Member

Fix would be to generalise the forward passes, and not have types on the input data formats.

(m::Parallel)(x) = mapreduce(f -> f(x), m.connection, m.layers)
(m::Parallel)(xs...) = mapreduce((f, x) -> f(x), m.connection, m.layers, xs)

@ToucheSir
Copy link
Member

@darsnack do you remember why the bound was put in place in the initial PR?

@darsnack
Copy link
Member

No idea why...seems like a mistake. We should definitely drop them.

@bhvieira
Copy link
Contributor

If it passes the tests, then it's fine

@DhairyaLGandhi
Copy link
Member

It passes - @rkurchin what in your opinion is the most sensible default for tuple inputs. Should the elements of the tuple be distributed across the branches of the Parallel layer, or should the tuple be sent wholesale to every branch. My thought would be - if one is wrapping the inputs in the tuple, it seems to me that they are indicating that it is a singular entity, but I would be curious about your thoughts.

@darsnack
Copy link
Member

One thing to note is that the current behavior for Chain muddies the water for Tuple and Vararg. We can't talk about tuple inputs to Parallel without considering tuple outputs from a previous layer. If I write a layer:

(m::MyLayer)(x) = f(x), g(x)

Then that naturally reads as MyLayer produces two outputs. It also matches the Julia semantics. This output will be passed as a single Tuple to the next layer by Chain. If the next layer is Parallel, then we might want each branch to process each output separately (for example for "join" layers). It makes sense that if the preceding layer produces multiple outputs, then the following layer should understands those as multiple inputs. On the other hand, if I write

(m::MyLayer2)(x) = tuple((f(x), g(x)))

This very explicitly says that MyLayer2 produces a single output that just happens to be a Tuple. Alternatively, the following also keeps the elements as a single entity, and it doesn't clash with Julia's semantics for multiple outputs == tuple:

(m::MyLayer3)(x) = [f(x), g(x)]

Unless we change the behavior of Chain, making the Tuple a single entity will mean we lose the ability for Parallel to process multiple arguments separately.

Didn't mean to derail: @rkurchin I'd still like to hear your thoughts too.


Also, I feel like this multiple arguments issue should be handled more carefully and separately. We can still merge #1674 now.

@rkurchin
Copy link
Author

rkurchin commented Aug 2, 2021

In my particular use case, what I want is more like the "zip" behavior (argument 1 goes to branch 1, argument 2 goes to branch 2), but I'm not sure if that ought to inform big-picture decisions here because it's just one specific application. I'm inclined to defer to @darsnack etc. on what the defaults ought to be, especially since the thing I'm working on right now doesn't even have the Chain issue, since the Parallel construct is the first one in the chain.

@DhairyaLGandhi
Copy link
Member

Zip like behavior seems to be the least surprising to me. We should keep with that, and document that mismatched lengths would behave exactly like they do with zip, so users should be careful. Probably not worth a warning in this case.

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 a pull request may close this issue.

5 participants