search

neurospin / pylearn-parsimony_history

Sparse and Structured Machine Learning in Python
BSD 3-Clause "New" or "Revised" License
0 stars 1 forks source link

`__init__` in some function interfaces #29

Closed duboism closed 10 years ago

duboism commented 10 years ago

Hi,

While working with Fouad, I have noticed that some interfaces define a __init__ function. Here is a list of those:

I'm not sure to understand why an interface should have a constructor.

In some cases this is clearly linked to issue #27. For instance, in our case (see the code of the RightSingular_SmoothedL1_L2_TV class in brainomics-team/2014_pca_tv/draft_code/pca_tv.py), we derive a new NesterovFunction because we want to use ExcessiveGap. We cannot call NesterovFunction constructor because it would need a A matrix which is not defined.

Such a constructor probably make more sense in OR but I'm not sure of the use of this class. Maybe this should be turned into some kind of factory.

tomlof commented 10 years ago

I think interface is the wrong name for what those are. Perhaps properties would be a better name?

Yes, this is a good catch! The idea here is that different interfaces should take care of the arguments that they require. Thus, the NesterovFunction interface takes its lagrange multiplier, the A matrix and some other things. And saves them in the object in a way that is required by the interface, not the function currently being implemented. This saves a lot of code in the functions, and makes it easier to use the interfaces in different algorithms and functions. Thus, the interfaces are more like base classes than interfaces and really should be renamed...

So, it should not be possible to create object of the interfaces, but in the implementing classes we should call the super constructors.

The OR interface is interesting. All algorithms have a list of interfaces that the provided function must implement, so that the algorithm knows that the function has all the methods it requires. However, in some algorithm I could accept either interface A or interface B, and there would be no difference. So, instead of having two algorithms do the same thing for two (in the current context) equivalent interfaces, I added the OR interface, so that I can say interfaces.OR(interfaces.A, interfaces.B) and both would work. Kind or neat, right? And RPN, no less! ;-)

tomlof commented 10 years ago

I propose the following change:

functions/interfaces.py -> functions/properties.py

In addition, I propose the following changes for the algorithms:

algorithms/explicit.py --> algorithm/gradient.py  (GradientDescent)
                       `-> algorithm/primaldual.py  (CONESTA, ExcessiveGapMethod)
                       `-> algorithm/proximal.py  (ISTA, FISTA)
                       `-> algorithm/utils.py  (Bisection, etc.)
algorithms/implicit.py --> algorithm/nipals.py  (FastSVD, FastSparseSVD, FastSVDProduct)

Let me know what you think!

duchesnay commented 10 years ago

+1

tomlof commented 10 years ago

Ok, I've made this changed and pushed to main repo. Closing this issue now.