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

Initialization of NMF parameters #1

Open
marianotepper opened this issue Jun 18, 2016 · 1 comment
Open

Initialization of NMF parameters #1

marianotepper opened this issue Jun 18, 2016 · 1 comment

Comments

@marianotepper
Copy link
Member

marianotepper commented Jun 18, 2016

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
Copy link
Member Author

marianotepper commented Jun 20, 2016

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.

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

1 participant