search

sensein / mapalign

Diffusion map based embedding and alignment
Other
68 stars 28 forks source link

fail on import of spectral_embedding_ #20

Open steelec opened 2 years ago

steelec commented 2 years ago

We were testing your package out and noted that it works with some versions of sklearn but not others. This appears to be due to the import statement in embed.py included below. I have not had a lot of time to track it down, but I think direct importing of these functions was removed in sklearn v .24 forward?

>> from sklearn.manifold.spectral_embedding_ import _graph_is_connected

FutureWarning: The sklearn.manifold.spectral_embedding_ module is  deprecated in version 0.22 and will be removed in version 0.24. The corresponding classes / functions should instead be imported from sklearn.manifold. Anything that cannot be imported from sklearn.manifold is now part of the private API.
  warnings.warn(message, FutureWarning)

https://github.com/satra/mapalign/blob/907d6c443e099fdb282996c1768c895eb13f300f/mapalign/embed.py#L73

satra commented 2 years ago

given python, i don't think they can make things really private. but the module name changed from sklearn.manifold.spectral_embedding_ to sklearn.manifold._spectral_embedding (https://github.com/scikit-learn/scikit-learn/blob/main/sklearn/manifold/_spectral_embedding.py). we may have to try both imports to account for pre and post versions, or simply change it the latter and require sklearn 1.0 or greater (perhaps better).

do you think you could send a PR for this ?

steelec commented 2 years ago

Hopefully this is what you meant, PR here #21