-
Notifications
You must be signed in to change notification settings - Fork 19.5k
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
Extending multi-io support #1051
Conversation
I like the idea seriously, it is more intuitive then the current API even the MultiLayer is a good idea since we are going to have lot of them (At least all the layers i have implemented in my library follows this idea :D ) |
nice @elanmart I will check this out soon! You also made me remember its time to resume working on the recurrent container. |
@EderSantana I thought about it a lot :) for now i'm implementing attentive models with huge repeat vectors, Time distributed merge and Time distributed dense :) Fairly inefficient ^_^ |
@@ -239,44 +246,59 @@ def add_input(self, name, input_shape, dtype='float'): | |||
'input_shape': input_shape, | |||
'dtype': dtype}) | |||
|
|||
def add_node(self, layer, name, input=None, inputs=[], | |||
merge_mode='concat', concat_axis=-1, dot_axes=-1, create_output=False): | |||
def add_node(self, layer, name, input=None, merge_mode='concat', concat_axis=-1, create_output=False): |
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.
This is another change in API and will break some models. Will need docs and public announcements.
Can you leave old inputs
for a while for compatibility purposes? We could mark it for deprecation and give the users a month or so to adapt.
@elanmart I didn't understand (but that is my fault) how a TODOs:
|
@EderSantana Thanks for comments!
Yes, but note that g = Graph()
g.add_input('X', (100,))
g.add_input('att', (None, 250))
g.add_node(Attention(256), 'attention', input={'state':'X', 'attended':'att'}, create_output=False)
g.add_node(Dense(128), 'd', input='attention', create_output=True)
g.compile(Adam(), {'d':'mse'})
Well, without this the The way it is done now is what I found most convienient: |
got it, thanks for the clarifications! |
if n in self.nodes: | ||
to_merge.append(self.nodes[n]) | ||
elif n in self.inputs: | ||
to_merge.append(self.inputs[n]) | ||
else: | ||
raise Exception('Unknown identifier: ' + n) | ||
merge = Merge(to_merge, mode=merge_mode, concat_axis=concat_axis, dot_axes=dot_axes) | ||
raise Exception('Unknown identifier: ' + n) |
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.
Unnecessary whitespace. Please install a PEP8 linter to spot these issues during development.
Agreed, that's better.
Can you expand on that? |
@fchollet thanks for comments, I'm gonna fix all those small issues ASAP if the main idea gets approved.
As in #620(comment) model.add_node(layer, input={'my_layer_input_1': 'node_name_1', 'my_layer_input_2': 'node_name_2'}) only that |
I believe it's a good idea. Since such an interface is assumption-heavy, making the UX work smoothly will require very thorough assumption checking and helpful error messages. |
@fchollet what about the |
@EderSantana @elanmart Whats the recurrent container? |
See #620 @EderSantana perhaps we could team-up on this one too, I've got some ideas that I think can be quite usefull. |
Oh yeah @farizrahman4u and @elanmart lets do this!!! My sample code worked on the pre-shape inference Keras. But now with the new modifications and so many people familiar with Keras source, it can be much easier to do it. I could make a PR with the old code and update it from there or we could start a new one from scratch. At least we have an API idea now, the implementation will just flow... We have a chance to build the best and most intuitive way to design RNNs. Totally abstracting for-loops whatsoever. Let us continue this conversation on #620 So we don't @elanmart 's PR. |
Any progress on this issue? |
Closing outdated PRs following the release of Keras 1.0. |
Extends #1018 and #1035.
The PR includes:
Graph
API (only oneinput
argument, modifyset_previous
and allow more flexible connectivity using multi-io layersMultiInputOutputLayer
(name very much [WIP])The goal was to allow arbitrary connectivity between layers inside a
Graph
.I think it is more intuitive than
join
mode inside aMerge
.Also, the new
MultiInputOutputLayer
has two additional advantages: it replicatesGraph
API and allows users to easily create new layers.Below you can find a simple example of using the new API to build an
attention
module (will become usefull as soon as we figure out how to write aRecurrent
container):ping @EderSantana I'd love to hear Your opinion.