Placement of if clauses

[f0k]

Nice idea! I'm not so sure about the current implementation pattern, though. Take tensor.nnet:

from ..config import is_theano, is_cgt
if is_theano():
    import theano.tensor.nnet
else:
    import cgt
    import cgt.nn

def sigmoid(x):
    if is_theano():
        return theano.tensor.nnet.sigmoid(x)
    else:
        return cgt.sigmoid(x)

def relu(x):
    if is_theano():
        return theano.tensor.nnet.relu(x)
    else:
        return cgt.nn.rectify(x)

def softmax(x):
    if is_theano():
        return theano.tensor.nnet.softmax(x)
    else:
        return cgt.nn.softmax(x)

In the very beginning, you already check which backend was selected, so there's no way out. So instead of checking for the backend again and again, you could just do:

from ..config import is_theano, is_cgt
if is_theano():
    from theano.tensor.nnet import sigmoid, relu, softmax
elif is_cgt():
    from cgt import sigmoid
    from cgt.nn import rectify as relu, softmax
del is_theano, is_cgt

Similarly, if you have to wrap something (instead of just providing some existing function under a new name), you could place the defs in the initial if clauses.

I understand that most functions won't be called a lot anyway as they're just building the graph (so performance is not an issue), but it seems weird to replicate the clauses everywhere.

Are there other reasons for the current pattern? For example, do you plan to add your own docstring for each function? Do you want to fix the interface of the wrapper functions (e.g., relu is just a subset of Theano's relu now)?

[dementrock]

That's a very good point! I could definitely save many keystrokes if I thought of it earlier :)

There might be a few places where separate docstring is useful, especially when only partial functionality is supported in TensorFlow. But overall just checking the clause at the beginning will definitely make things much cleaner.

I think an even better way to organize the project would be to have separate "implementation" modules, e.g.

theano_impl/
    tensor/
        ...
cgt_impl/
    ...
tensorflow_impl/
    ...

And then selectively import them from the root

[f0k]

There might be a few places where separate docstring is useful, especially when only partial functionality is supported in TensorFlow. But overall just checking the clause at the beginning will definitely make things much cleaner.

If you want separate docstrings (that possibly list the constraints to be aware of depending on the backend), then probably the current pattern is the way to go.

I think an even better way to organize the project would be to have separate "implementation" modules

Sure, that would be cleaner, but if you spread things over multiple files like this, it may be harder to keep their interfaces in sync. Whenever you extend/change the interface, you need to touch three files instead of one then. It would limit the case distinction to your global __init__.py, though:

import os
mode = os.environ.get('TENSORFUSE_MODE', 'cgt')
del os
if mode == "theano":
    import * from .theano_backend
elif mode == "cgt":
    import * from .cgt_backend
elif mode in "tensorflow", "tf":
    import * from .tensorflow_backend
else:
    raise ImportError("Unsupported TENSORFUSE_MODE")

(That's against PEP8, but there are ways around it.)

[dementrock]

Right. I think I'm fine with maintaining multiple files, since most of the code is just reimporting (especially for theano), and most of the file content will focus on bridging the few incompatibilities.

Things get a little tricky when multiple backends share the same / similar bridging, like theano.scan. But these cases are rare so far.

Just wondering, if you do the import * in __init__.py, would import tensorfuse.tensor as T be resolved to the right module?

[f0k]

Just wondering, if you do the import * in __init__.py, would import tensorfuse.tensor as T be resolved to the right module?

Hmm, no. from tensorfuse import tensor as T would work, but for import tensorfuse.tensor as T you'd need a separate tensor.py with boilerplate.

Another advantage of providing the submodules would be allowing users to do:

import tensorfuse.cgt_backend as theano
T = theano.tensor

But still, if you want to be able to document each and every wrapper function, the current layout is suited better, otherwise you'd end up triplicating the docstrings.

[dementrock]

I do like the flexibility of importing custom backend. The replication of docstrings might not be that bad, since methods that need documentation will most likely have different specifications depending on the chosen backend. For example, for tensorfuse.compat.is_variable, under Theano it checks whether an object is an instance of theano.gof.Variable, and under TensorFlow it does something different to achieve the same semantics.

[f0k]

The replication of docstrings might not be that bad, since methods that need documentation will most likely have different specifications depending on the chosen backend.

It might be good to highlight those in a common docstring, though, so as a user you know what to expect in the different backends when using is_variable (without having to look at three docstrings). For every pro there's a con :)