search

mila-iqia / blocks

A Theano framework for building and training neural networks
Other
1.16k stars 351 forks source link

Make the Sequence brick implement the Sequence ADT. #1163

Closed dwf closed 2 years ago

dwf commented 8 years ago

This makes it a lot easier to do surgery on Sequence bricks. Say, for example, you have some code that generates a sequence and you want to customize it by adding or replacing layers; manually managing the .children and .application_methods attributes is a huge pain.

Marginally related to #1160.

dwf commented 8 years ago

Open question is whether __setitem__ and insert should auto-convert Bricks into b.apply to match #1160.

dwf commented 8 years ago

(The list of stuff we get from the MutableSequence inherit.)

rizar commented 8 years ago

Hmm, I sort of always assumed that the brick's children are fixed after they brick is created. Do you often modify them in-place?

dwf commented 8 years ago

Wouldn't say often, but for example I keep a lot of functions around that build standard-ish architectures and it's often a little simpler to just call that and then do some surgery before allocation for a slightly customized thing i want to try.

I'm surprised to hear you say that actually since deleting from the children list correctly handles deletion from the child's list of parents. :)

On Fri, Nov 11, 2016, 9:34 PM Dzmitry Bahdanau notifications@github.com wrote:

Hmm, I sort of always assumed that the brick's children are fixed after they brick is created. Do you often modify them in-place?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/mila-udem/blocks/pull/1163#issuecomment-260096301, or mute the thread https://github.com/notifications/unsubscribe-auth/AADrLgvuyLiT-SbJxuP8EQqlqZXIK6gyks5q9SW1gaJpZM4KvdoT .

rizar commented 8 years ago

The usecase that you refer to is valid. I didn't know that there is some support in the core for deletion of children, @bartvm wrote that part.

Would like to make self.layers of ConvolutionalSequence a property now? As you said, it makes sense if we assume that application methods can be removed.

dwf commented 8 years ago

df8a57e makes the change to ConvolutionalSequence. I also added the ability to use insert, etc. with Bricks, assuming this means "use the .apply method" as it now does with the application_methods argument to the constructor. __getitem__ always returns a BoundApplication though.

rizar commented 8 years ago

LGTM!

rizar commented 7 years ago

@dwf, looks like I approved this PR more than a month ago. Do you wish to proceed and merge it or should I close it?