Transformer #361

skaae · 2015-08-06T22:21:27Z

Spatial transformer Layer

TODO/questions

Tests. Added simple tests with https://github.com/skaae/nntools/commit/27faf05049c1889676ac2394d00b1b1677a72724
I get an error when generating the docs. It seems to be related to objectives.rst??
I put the TransformerLayer in a new file: transform.py. Do someone have a better suggestion?

skaae · 2015-08-07T02:29:59Z

I cleaned up the docs and added some very simple tests.

f0k · 2015-08-07T10:13:14Z

I get an error when generating the docs. It seems to be related to objectives.rst??

Oh, my bad. Didn't see that when doing #359, I only ran the tests :-/ Fixed in #364!

skaae · 2015-08-07T13:22:27Z

Thanks. I'll wait for #364 to be merged

f0k · 2015-08-07T13:42:57Z

Thanks. I'll wait for #364 to be merged

Merged! If you want, you can rebase your branch now:

git checkout transformer
git pull upstream master --rebase
git push --force origin transformer

skaae · 2015-08-07T13:46:45Z

And no doc errors, except for my own...

f0k · 2015-08-07T13:58:33Z

lasagne/layers/transform.py

+        The network that calculates the parameters of the affine
+        transformation. See the example for how to initialize to the identity
+        transform. Note that all parameters are clipped to lie in the range
+        ``[-1, 1]``, similar to [1]_.


Does that mean you're clipping the values in the code? Wouldn't it be more efficient to just require the localization_network to output values in [-1, 1] in the first place?

Yes you could do that. But if you happen to output values larger than -1,+1 i think you would get indexing errors which are pretty hard to debug.

But i'm very open to suggestions. Not sure if i have done it in the best way.

Maybe an option clip_transform=True that clips transformation parameters when activated (which should be the default), but that can be disabled for improved performance if your network has tanh output, for example.

I'm not sure, but i think that performance drop from using clipping is negligible?

Could be. Let's just leave that for later (i.e., a future PR, if it turns out to be worth it)!

skaae · 2015-08-07T18:28:40Z

Just realized that what if have said about the range of the affine parameters was not correct. The parameters are free to be any value. The clips are to ensure that we do not look up illegal indices.

I removed the references to [-1, +1] from the documetation. ]

f0k · 2015-08-08T13:31:27Z

lasagne/layers/transform.py

+    def get_output_shape_for(self, input_shapes):
+        shp = input_shapes[0]
+        dsf = self.downsample_factor
+        return shp[:2] + tuple(int(s//dsf) for s in shp[2:])


Just realized that this won't work with Nones. You'd need:

return (shp[:2] + tuple(None if s is None else int(s//dsf) for s in shp[2:]))

And maybe we should't use integer division if we allow floats for downsample factors?

For non integer downsample factor i just floor the output shapes. Is that the question?

f0k · 2015-08-08T13:39:54Z

Great, I like your documentation extensions! Just two minor things, and then I'd be all for squashing and merging!

skaae · 2015-08-08T14:45:11Z

I think this is done if any one else whant to have a look?

I would like to test the code on MNIST before I merge it. I have used the code at it worked fine, but just in case I have messed something up :)

f0k · 2015-08-10T08:55:06Z

lasagne/layers/transform.py

+    input : a :class:`Layer` instance
+        The input where the affine transformation is applied. This should
+        have convolution format, i.e. (num_batch, channels, height, width).
+    localization_network: : a :class:`Layer` instance


There's a colon too much (the one in localization_network:). Didn't see that before, sorry. Guess it might confuse Sphinx. Feel free to amend your commit if you have the time, otherwise we can leave that for some general docstring proofreading sometime.

f0k · 2015-08-10T09:00:16Z

Looks good except for one minor thing I've overlooked before, sorry!

I would like to test the code on MNIST before I merge it.

When you're testing again on MNIST, could you try replacing the integer // divisions by normal / divisions? It should be the same. The integer divisions suggest that downsample_factor would be an integer, which would render the type casting unneeded. So I'd prefer using / for the division if possible!

Let me know when you've tested on MNIST and are fine with this PR to be merged!

skaae · 2015-08-10T14:53:56Z

I fixed the last docstring error. Using / does work, which makes sense. Thanks for the detailed review :)

I'll get back when i have run the MNIST test. I'll probably add a recipe to reproduce the results in the paper.

f0k · 2015-08-10T15:46:03Z

I'll get back when i have run the MNIST test.

Cool! Feel free to commit / squash / push so Travis can run already. (Just to be sure, note that there are two occurrences of // in the code.)

f0k · 2015-08-13T12:20:52Z

Any news? Can we merge this today, or do you still want to test something?

skaae · 2015-08-13T13:31:17Z

No. Im still in Montreal but i’m going home today. I’ll be able to test it in the weekend.

On 13 Aug 2015, at 08:20, Jan Schlüter notifications@github.com wrote:

Any news? Can we merge this today, or do you still want to test something?

—
Reply to this email directly or view it on GitHub.

skaae · 2015-08-15T13:48:43Z

I ran a test on cluttered MNIST and the layer seems to be working fine. I'm fine with merging.

The experiment is here:

https://github.com/skaae/Recipes/blob/spatial_transform/examples/spatial_transformer_network.ipynb

The last plot shows that its does zoom as expected.

f0k · 2015-08-17T11:16:51Z

Cool, thanks a lot! Will merge this after #382. Your recipe looks very nice as well.

f0k · 2015-08-17T15:36:15Z

Thinking again, didn't we want to move that to special.py (#373 (comment))? If so, you'd need to rename transform.py, test_transform.py and docs/layers/transform.rst accordingly, and update the references in layers/__init__.py and docs/layers.rst. In #373, we've chosen "Special-purpose layers" for the header in docs/layers/special.rst: https://github.com/Lasagne/Lasagne/pull/373/files#diff-5e96c229ca5e2f81af27a8a50443dec8R1

Do you want to rename it here (and squash or amend), or shall we merge #373, rebase yours and move it? I guess the former would be less work for you, then I can care about rebasing #373.

skaae · 2015-08-17T15:45:54Z

Ah yes forgot that. Go ahead and merge #373. I can rebase this when you have merged #373.

f0k · 2015-08-17T16:12:25Z

All right, merged it. Rebasing this one should just trigger trivial conflicts in docs/modules/layers.rst and lasagne/layers/__init__.py. I guess it will be easiest to not add transform to those two files at all when you solve the conflict, then move over your code from *transform.* to *special.* and amend your commit.

skaae · 2015-08-17T16:42:03Z

I moved it to special.py . Hopefully Travis passes :)

f0k · 2015-08-17T16:46:53Z

Cool, that was fast! Looks good to me, let's see what Travis and Coveralls think!

f0k · 2015-08-17T17:16:19Z

Merging, thanks a lot!

We can now figure out if there are ways to make it faster if the image shape is known at compile time (which is probably a common use case).

Transformer

skaae mentioned this pull request Aug 7, 2015

spatial transformer network layer #355

Closed

skaae closed this Aug 7, 2015

skaae reopened this Aug 7, 2015

f0k reviewed Aug 7, 2015
View reviewed changes

f0k reviewed Aug 8, 2015
View reviewed changes

f0k reviewed Aug 10, 2015
View reviewed changes

skaae mentioned this pull request Aug 13, 2015

added UnLayer to base.py and tests #373

Merged

TransformerLayer

ecc1e1d

f0k added a commit that referenced this pull request Aug 17, 2015

Merge pull request #361 from skaae/transformer

db57650

Transformer

f0k merged commit db57650 into Lasagne:master Aug 17, 2015

skaae mentioned this pull request Aug 17, 2015

Spatial transform Lasagne/Recipes#7

Merged

skaae deleted the transformer branch August 20, 2015 17:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Transformer #361

Transformer #361

skaae commented Aug 6, 2015

skaae commented Aug 7, 2015

f0k commented Aug 7, 2015

skaae commented Aug 7, 2015

f0k commented Aug 7, 2015

skaae commented Aug 7, 2015

f0k Aug 7, 2015

skaae Aug 7, 2015

skaae Aug 7, 2015

f0k Aug 7, 2015

skaae Aug 7, 2015

f0k Aug 7, 2015

skaae commented Aug 7, 2015

f0k Aug 8, 2015

f0k Aug 8, 2015

skaae Aug 8, 2015

f0k commented Aug 8, 2015

skaae commented Aug 8, 2015

f0k Aug 10, 2015

f0k commented Aug 10, 2015

skaae commented Aug 10, 2015

f0k commented Aug 10, 2015

f0k commented Aug 13, 2015

skaae commented Aug 13, 2015

skaae commented Aug 15, 2015

f0k commented Aug 17, 2015

f0k commented Aug 17, 2015

skaae commented Aug 17, 2015

f0k commented Aug 17, 2015

skaae commented Aug 17, 2015

f0k commented Aug 17, 2015

f0k commented Aug 17, 2015

Transformer #361

Transformer #361

Conversation

skaae commented Aug 6, 2015

skaae commented Aug 7, 2015

f0k commented Aug 7, 2015

skaae commented Aug 7, 2015

f0k commented Aug 7, 2015

skaae commented Aug 7, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skaae commented Aug 7, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

f0k commented Aug 8, 2015

skaae commented Aug 8, 2015

Choose a reason for hiding this comment

f0k commented Aug 10, 2015

skaae commented Aug 10, 2015

f0k commented Aug 10, 2015

f0k commented Aug 13, 2015

skaae commented Aug 13, 2015

skaae commented Aug 15, 2015

f0k commented Aug 17, 2015

f0k commented Aug 17, 2015

skaae commented Aug 17, 2015

f0k commented Aug 17, 2015

skaae commented Aug 17, 2015

f0k commented Aug 17, 2015

f0k commented Aug 17, 2015