Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
PairwiseFusion
layer, take 2 #1983PairwiseFusion
layer, take 2 #1983Changes from 9 commits
6a2d8de
5c78e13
98e6e5f
96a6448
617ae2a
1180a62
e842e7b
167d5d4
121b898
8e2e840
cf758eb
5346e78
78157e3
d0f0a29
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be good to add a concrete example
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's one in the tests, but I was holding back because I wanted to take time later to come up with a better example
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this, like Parallel, allow
m(x1, x2, x3) == m((x1, x2, x3))
?I also wonder if the one-
x
case should bex::AbstractArray{<:Number}
, or something. So that we don't find out someone is relying on some unintended behaviour, e.g. how a NamedTuple is handled. Although Parallel does not make such a restriction.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first suggestion seems okay. The second would make the layer less usable if the sub-layers are custom layers that accept something other than the type restriction that we provide. Presumably, in most cases, the sub-layers should be appropriately restricted to throw an error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've allowed the first. Not sure about the second...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see the method allowing this:
Agree that the second is less obvious. In particular it rules out many easy readme examples.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops, yeah, I think I'd missed pushing that - should be fixed now