lightonai / lightonml

Python library for running large-scale computations on LightOn's OPUs
https://docs.lighton.ai
Other
35 stars 11 forks source link

Separated Bit Plan encoder fit / transform behaves un-intuitively #6

Open lucgiffon opened 2 years ago

lucgiffon commented 2 years ago

Hello,

In the scikit-learn framework, I believe the fit / transform methods are coded with the following philosophy:

Or at least this is how I see things.

But objects of the SeparatedBitPlanEncoder class from the lightonml library doesn't behave like this. Instances of this class change their inner state when we call their transform method. I find this misleading because it makes no difference between the transform method and the fit_transform one.

The consequence is that we cannot instanciate one SeparatedBitPlanEncoder and one SeparatedBitPlanDecoder once and for all and then use the same for all the OPU calls ! I can see from here the huge problems such a thing could cause !!!

At the very least, the transform method of SeparatedBitPlanEncoder should be disabled and only fit_transform should remains. This way, it would be clear that we change its state when using it. But an even better improvement would be to have a real fit method that actually fits to its input and then a transform method that would raise an exception if the input values are out of the range of values it is capable to handle.

I really need this feature so I am going to implement it for myself. If you want, I can make a PR when it is done.

Bye

iacolippo commented 2 years ago

Hello Luc, a PR would certainly be welcome!