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

Split up layers/base.py #68

Closed
f0k opened this issue Dec 14, 2014 · 9 comments
Closed

Split up layers/base.py #68

f0k opened this issue Dec 14, 2014 · 9 comments
Milestone

Comments

@f0k
Copy link
Member

f0k commented Dec 14, 2014

nntools.layers currently has the following structure:

  • corrmm.py: Conv2DMMLayer (2D convolution using GpuCorrMM)
  • cuda_convnet.py: Conv2DCCLayer, MaxPool2DCCLayer, ShuffleBC01ToC01BLayer, ShuffleC01BToBC01Layer (2D convolution and pooling using pylearn2's cuda-convnet wrappers)
  • base.py: all other layer classes we have
  • __init__.py: imports * from base and imports the two other submodules

base.py is a very long file with several different kinds of layers. I propose to split it up into smaller files that each hold a group of layers that belong together. __init__.py should still import those layers directly, so one doesn't have to remember which group a layer belongs to. A possible layout might be:

base.py:

__all__ = [  # so "from .base import *" only imports the relevant names
        'Layer',
        'MultipleInputsLayer', 
        'InputLayer',
        ]

class Layer(object): ...
class MultipleInputsLayer(Layer): ...
class InputLayer(Layer): ...  # not really a base class, but probably does not warrant its own submodule

helper.py: get_all_*, set_all_*
?.py: DenseLayer, NINLayer
noise.py: DropoutLayer, GaussianNoiseLayer
conv.py: Conv1DLayer, Conv2DLayer, [Conv3DLayer]
pool.py: MaxPool2DLayer, FeaturePoolLayer, FeatureWTALayer, GlobalPoolLayer
?.py: FlattenLayer, [ReshapeLayer, ReshuffleLayer], PadLayer?
merge.py: ConcatLayer, ElemwiseSumLayer

There are some questions marks, but that's about the grouping I see (some of these groups are already formed in the existing base.py, some are a little different). Any comments, refinements, suggestions? Let's try to converge on a good set of categories and split it up!

@benanne
Copy link
Member

benanne commented Dec 14, 2014

Looks good to me. I would call the first ? 'dense.py' and the second one 'shape.py' or something. Since these names are not really going to be exposed to the user, they're not really that crucial to get 'right' imo. Short is good :)

On that note, we should probably still make a distinction between the 'base' set of layers, that are available in the nn.layers namespace, and the more specialized stuff that is only accessible through the submodules (like nn.layers.cuda_convnet, nn.layers.corrmm).

I believe all the layers you listed are probably important enough to live in the main namespace, but we'll have to keep this in mind and make a decision when we add in the recurrent layers for example.

@craffel
Copy link
Member

craffel commented Dec 14, 2014

I think splitting up base.py is definitely a good idea, but I'd propose that we give access to all of the layer classes straight from nntools.layers (e.g. DropoutLayer would be nntools.layers.DropoutLayer instead of nntools.layers.noise.DropoutLayer) - I don't think there will be any namespace conflicts (e.g. something in some layer submodule which is not in another), and I think it will save a lot of typing. I think this is what you just proposed @benanne; I guess what I'm saying is I think all layers are probably important enough to live in the main namespace.

@benanne
Copy link
Member

benanne commented Dec 14, 2014

Yep, that's the idea. But for specialized stuff like the cuda_convnet and cormm layers, I don't think it's necessary. Are you saying you think these should also be available in the main namespace?

@craffel
Copy link
Member

craffel commented Dec 14, 2014

I think that as long as there aren't any namespace conflicts (e.g. a layer with the same name in cuda_convnet and conv), we may as well put them all in the same namespace.

@benanne benanne added this to the First release milestone Dec 18, 2014
@benanne
Copy link
Member

benanne commented Dec 20, 2014

I was thinking about doing this quickly, but clearly there are a lot of dependencies between future submodules. How should these be handled?

It seems cleanest to just import those submodules where necessary and use them directly. But this will make the code a bit harder to read, because it is less like 'normal' code that a user would write (i.e. a user would write layers.DenseLayer, or just DenseLayer, but never dense.DenseLayer).

The alternative is to import * the necessary submodules (since that is what happens in __init__.py anyway).

What's the best option? We should probably pick one of these and do that consistently across the library.

@f0k
Copy link
Member Author

f0k commented Dec 25, 2014

I think the best option is yet another way: Importing just the names that are needed, to be explicit about the dependencies. E.g., if the recurrence module depends on dense.DenseLayer, it should do from .dense import DenseLayer. This way the code still reads like 'normal' code, as you called it, and we avoid the namespace mess created by import *.

@benanne
Copy link
Member

benanne commented Dec 25, 2014

Yes! Let's do that :)

@benanne
Copy link
Member

benanne commented Dec 26, 2014

I'll try to sort this out today.

@benanne
Copy link
Member

benanne commented Dec 26, 2014

This was done in #81.

@benanne benanne closed this as completed Dec 26, 2014
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

No branches or pull requests

3 participants