-
Notifications
You must be signed in to change notification settings - Fork 184
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
Make API more inline with scikit-learn #33
Comments
KeplerMapper's API has not evolved since its first iteration. Back then I only clustered on the projection and it was more of a single-function-does-everything. Right now, I dont even know if fit_transform is the right term to use (you are essentially projecting). Seperating out fit, transform, and fit_transform would allow for mapping on unseen data (calculate in which interval the new data falls, and add to most relevant cluster). For small changes like nr_cubes -> n_cubes, if not too much added complexity, id like for this to accept both (non-breaking changes for old code, and updated documentation). Afaik, scikit-learn does not do graphs at all. The closest data structure is with http://scikit-learn.org/stable/modules/generated/sklearn.cluster.AgglomerativeClustering.html but this uses keyed Numpy arrays (probably forced, to be consistent with other structures/data representations/optimizations). I think either dicts or NetworkX objects are more flexible to experimentation (add link strengths, add link functions). With NetworkX I think it also becomes easier to run graph algorithms (shortest or longest paths) and to compare Gromov-Hausdorff distance between graphs. As for the visualization side, I can accept both dicts and NetworkX objects. Numpy arrays would become a bit more difficult for me. |
I'm still new to the scikit_learn API, but it seems fit/fit_transform is the main work horse of each algorithm. Because of this I think map should be renamed to fit. We could roll current map functionality into fit_transform and then allow users to provide a custom lens or a projection:
or
Maintaining backwards compatibility is great, but we shouldn't try to keep it around forever. Maybe we should start dev'ing on a new branch and release backwards breaking changes with 1.2 or 2.0? It would be best to release all API changes at the same time, so letting them sit while we fine-tune them would be ideal.
Let's stick with dicts and networkx objects then. Is it currently wired to accept networkx objects? Adding networkx as a requirement seems okay to me. One thing I'd like to work on soon is adding ability for a custom linker. I'd like to be able to build arbitrary dimensional simplicial complexes, and this could also provide an interface for specifying the type of output. I think this will also be an essential component for implementing multi-mapper (a custom cover scheme and associated linker to handle the filtration). |
In my workflow I found it useful to call .fit_transform multiple times. I also like that behind the scenes it calls .fit_transform and that the output is a Numpy array. First impression is to not consolidate .map and .fit_transform. This seems more friendly to custom (or chained) projections. It should also be able to feed inverse_X=None and apply clustering on the projection (suffer projection loss), for instance in the case where it is possible to create a lens from the data (final layer -> t-SNE(2-d)), but not possible to cluster on the original data (clustering on raw pixel values fails to provide meaningful clusterings). But I don't have a strong opinion about this and need to think about this more (and incorporate feedback from using kmapper on many varied data sets). |
I'll write a networkX -> dict convertor which kicks in if you don't pass a dict, but an object. Lets keep NetworkX optional (required only if you set parameter to output a NetworkX object). |
Regardless of consolidating projecting and mapping, it would be cool to be able to pipeline lenses.
|
That would be a nice feature, and shouldn't be very difficult to dev. Is there any way we could incorporate chaining of function calls into the API? This would make the calls look something like:
This would be a bit more far-fetched and would require that Either way, the API you showed should work without major changes. |
I like how so much can now fit inside a single tweet: import kmapper as km
from sklearn.datasets import make_circles as c
from sklearn import manifold
inverse_X, y = c(3001)
K = km.KeplerMapper()
projected_X = K.fit_transform(inverse_X, projection=manifold.TSNE(3))
G = K.map(projected_X, inverse_X)
K.visualize(G, custom_tooltips=y) I liked being able to call If we see I see no objection in having another function where you do more things in one go (perhaps more like in a Pipeline manner or whatever suits your research). |
Wanting something like:
Where the projected_X is created by 5-fold out-of-fold guessing (we can write own code for this, or wait for the upcoming StackingEstimator). This gets us lens support for all Scikit-Learn API-compatible supervised and unsupervised models. You could run a quick XGBoost on the data and use this with the ground truth as a color function to see where tree-based models perform poorly (and need better feature engineering). |
Also, separating out fit_transform would make it easier to pickle?
|
I like being able to skip importing commonly used sklearn libraries like I think we can use:
if we add We could also have a custom
https://github.com/baidu/fast_rgf Although perhaps a bit heavy on the Machine Learning-side (dont want to shoehorn KeplerMapper into an auto-ML library), it would make KeplerMapper fully competitive with state-of-art supervised learning. |
💭
|
We'd like all idioms used in KeplerMapper to fit seamless with the scikit-learn API.
What needs to be changed? Below are some things that have been brought up before and questions to ask about the current API.
fit
method and havefit_transform
only runfit
and return the result.map
method fit into this design? Shouldmap
befit
instead?Please suggest other changes to the API!
The text was updated successfully, but these errors were encountered: