scicubator / nimfa

nimfa - A Python Library for Nonnegative Matrix Factorization Techniques
http://nimfa.biolab.si
0 stars 1 forks source link

Initialization of NMF parameters #1

Open marianotepper opened 8 years ago

marianotepper commented 8 years ago

All the different methods initializers are calling the initializer of the parent class. For example, here: nmf_std.Nmf_std.__init__(self, vars()) This is done in all the different NMF flavors, each calling the corresponding initializer.

I have two comments about this way of initializing things: 1) Wouldn't it be more pythonic to use super instead? It would also reduce the possibility of calling the wrong initializer by mistake.

2) vars() contains a reference to self, so in practice every instance contains a self-reference (self.self) after initialization. Although it does not hurt, I believe that this reference should be eliminated from the vars dictionary. This can be done in each subclass, e.g.,

        params = vars()
        del params['self']
        nmf_std.Nmf_std.__init__(self, params)

It can also be done in the base class itself (Nmf_std in this case) to avoid code repetition.

I believe that making these simple and quick changes would improve object initialization in the library.

marianotepper commented 8 years ago

Another problem with the initializers is the following one. Sub-classes must define the member variables 'name' and 'aseeds' BEFORE calling the super-initializer, because these are needed during the execution of the latter. For example: https://github.com/scicubator/nimfa/blob/master/nimfa/methods/factorization/nmf.py#L160 This creates a weird mutual dependency between the parent and its sub-classes.

I believe the code is conceptually cleaner if these are passed as parameters of the super-initializer. Here is an example of how I suggest to do this: https://github.com/scicubator/nimfa/blob/master/nimfa/methods/factorization/sepnmf.py#L99

Do you agree that this is a problem? Alternative solutions and/or suggestions are welcome.