Adding Gamma poisson factorization

[alexis-cvetkov]

Changes:

• Added gamma_poisson_factorization.py which implements online Gamma-Poisson factorization for encoding string variables.
• Added the corresponding tests intest_gamma_poisson_factorization.
• Modified examples 02 and 03 to include this method.
• Updated CHANGES.rst and index.rst to describe this new method.

[GaelVaroquaux]

Wohoo!!

@Phaide : can you have a look when you have time?

Have a look at the code, the test (it's OK if you don't understand everything, this is a sophisticated mathematical algorithm. Check that you understand how to use this, that it feels natural, that the examples make sense, that it's well referenced in the docs (we should point from SimilarityEncoder to this for large datasets). Thanks!!!!

[alexis-cvetkov]

Following the error on travis, the sklearn version is checked when importing the "k-means++" init method and the _beta_divergence method to account for different file and method names across versions.

[LilianBoulard]

Very good !
Thanks @alexis-cvetkov, good work !

The code is clean and fairly documented (though a few more inline comments to explain complex operations would be welcome!).
One last request: as mentioned by Gaël, could you please consider adding a little notice in the SimilarityEncoder docstring ?
Thanks again !

[amy12xx]

Great to see this addition. I look forward to going through the code when I have a bit more time.

[alexis-cvetkov]

Thank you for the review @Phaide, I have added more documentation in the code for gamma-poisson factorization, and a little docstring in similarity_encoder.py.
This PR looks ready for merge to me now !

[LilianBoulard]

Looks good to me !
@amy12xx could you please have a quick look before merging ?
Thanks !

[amy12xx]

This looks great @alexis-cvetkov . I left a few minor comments. And one other general comment: I wonder if it is a good convention to have the referenced methods above the referenced ones (for e.g. the non-class functions before the class definition).

[GaelVaroquaux]

Here are a bunch of cosmetic comments.

I'll try to come back and check the algorithmics, but we had checked them back when we coded this originally.

Thanks for the hard work!

[GaelVaroquaux]

Here is an attempt: Added online Gamma-Poisson factorization through the :class:`GapEncoder` class. This method discovers latent categories formed via combinations of substrings, and encodes string data as combinations of these categories. To be used if interpretability is important.

This is great!!

[GaelVaroquaux]

This test takes about 40s to run, and less if I decrease the number of samples/ components. Should it be done on only a fraction of possible combinations then ?

Yes please. I'm not sure that being exhaustive is crucial. I'd rather work on a smaller number of well-chosen combinations. I would keep the number of samples, so that the test is reliable.

[alexis-cvetkov]

CircleCI is not building anymore. I think this is because it now uses python 3.9 during the build, and some packages like scikit-learn do not support it yet.

Update: I pinned the version of python to 3.8 in build_doc.sh. I don't know if it's the right way to fix this but it works!

[amy12xx]

Regarding my list comprehension comment, I had something like this in mind: self.H_dict = {x: h for x, h in zip(X, H)} but I think I assumed that H_dict would have the exact same elements as X. If this is not the case, you can ignore this comment.

[GaelVaroquaux]

Decreasing the batch_size to 100 solves this,

Good idea: try putting it to 128. Thanks!

[amy12xx]

Thanks @alexis-cvetkov . Looks good to me.

[GaelVaroquaux]

Thanks a lot for the PR. I finally had time to review it in depth.

I made a bunch of inline comments. On top of these, I have a few bigger picture comments:

• We need to add a comment at the top of the "Scalability considerations" example saying that the GapEncoder is naturally scalable and should be preferred for large-scale data.
• We should restore the get_feature_names method and demo it in one of the examples
• We should restore the partial_fit method and demo it in a new example on scalability

Thanks!!!

[GaelVaroquaux]

Is there a specific reason to initialize H_dict in fit rather than in __init__ ?

It implies that every call to fit has a clean start, independent of the past.

Also, it means that multiple calls of fit will update the same H_dict and not retrain the model from scratch. Is that what we want ?

I'm confused: isn't it the other way around: initialize in fit means that multiple calls of fit will not update the same H_dict (partial_fit should behave differently), no?

[alexis-cvetkov]

It implies that every call to fit has a clean start, independent of the past.

Okay, I was confused by the test you proposed:

if not hasattr(self, "H_dict"):
    ...

In this case I don't need to test if H_dict exist but I can simply re-initialize it everytime fit is called
For partial_fit however I can use this test to init H_dict once and update it for the following calls of partial_fit.

[GaelVaroquaux]

Okay, I was confused by the test you proposed:

OK, this was an error on my side. Thanks for noting it.

[GaelVaroquaux]

@alexis-cvetkov : careful, I am pushing to your branch. If you work on this, pull first.

[GaelVaroquaux]

I believe that this is now good to go. I will merge when the tests have run. Thanks!!!

[alexis-cvetkov]

Great ! Thank you !