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

[MRG] DOCATHON : LLE and Isomap match doc for sparse input #8554

Merged
merged 10 commits into from Jul 2, 2018

Conversation

Projects
None yet
4 participants
@lmcinnes
Contributor

lmcinnes commented Mar 7, 2017

Fix #8416

Isomap does, in fact support sparse input. Code was changed to match the documentation, and a test was added to verify that it doesn't error.

LLE does not support sparse input -- the barycenter_kneighbors_graph function makes use of fancy indexing which is not supported by sparse matrices. Further the claim of support for KDTree, BallTree and NearestNeighbor objects turned out to be false as well. Docs were updated to reflect this.

@lesteve

This comment has been minimized.

Show comment
Hide comment
@lesteve

lesteve Mar 8, 2017

Member

Your history looks a bit weird, e.g. you have some commits from your work on t-SNE perplexity. Can you fix that?

Member

lesteve commented Mar 8, 2017

Your history looks a bit weird, e.g. you have some commits from your work on t-SNE perplexity. Can you fix that?

@lesteve

This comment has been minimized.

Show comment
Hide comment
@lesteve

lesteve Mar 8, 2017

Member

Please use "Fix #issueNumber" this way the associated issue gets closed automatically when the PR is merged. For more details, look at this.

Note this pattern is not flexible so using "Fix issue #issueNumber" or "Fix for #issueNumber" is not going to work. I have edited your description.

Member

lesteve commented Mar 8, 2017

Please use "Fix #issueNumber" this way the associated issue gets closed automatically when the PR is merged. For more details, look at this.

Note this pattern is not flexible so using "Fix issue #issueNumber" or "Fix for #issueNumber" is not going to work. I have edited your description.

@lmcinnes

This comment has been minimized.

Show comment
Hide comment
@lmcinnes

lmcinnes Mar 8, 2017

Contributor

Sorry about that -- apparently I got my branching mixed up and had a separate patch carried into this. Should be good now.

Contributor

lmcinnes commented Mar 8, 2017

Sorry about that -- apparently I got my branching mixed up and had a separate patch carried into this. Should be good now.

@lmcinnes

This comment has been minimized.

Show comment
Hide comment
@lmcinnes

lmcinnes Jun 23, 2018

Contributor

Is this outdated now by other changes? If so please feel free to close it. If not I would appreciate a review if and when time is available. Thanks.

Contributor

lmcinnes commented Jun 23, 2018

Is this outdated now by other changes? If so please feel free to close it. If not I would appreciate a review if and when time is available. Thanks.

@lesteve

This comment has been minimized.

Show comment
Hide comment
@lesteve

lesteve Jun 27, 2018

Member

Thanks for the friendly ping, LGTM. If someone that is more familiar with the manifold code than me can have a look, in particular double-check #8554 (comment),
it would be great!

Member

lesteve commented Jun 27, 2018

Thanks for the friendly ping, LGTM. If someone that is more familiar with the manifold code than me can have a look, in particular double-check #8554 (comment),
it would be great!

@lesteve

This comment has been minimized.

Show comment
Hide comment
@lesteve

lesteve Jun 29, 2018

Member

I can confirm this is not working for me with a *Tree, but is working with a NearestNeighbors. Surely we should have a test to that effect?

I updated the docstrings to a version that seems to be the consensus, i.e. array-like or NearestNeighbors.

I quickly run the tests with a print statement and we do have tests for locally_linear_embedding that takes X as a NearestNeighbors object.

I have no idea about the CircleCI failure but I would just ignore this one.

@jnothman should we had a whats_new entry that says that Isomap can now fit a sparse matrix?

Member

lesteve commented Jun 29, 2018

I can confirm this is not working for me with a *Tree, but is working with a NearestNeighbors. Surely we should have a test to that effect?

I updated the docstrings to a version that seems to be the consensus, i.e. array-like or NearestNeighbors.

I quickly run the tests with a print statement and we do have tests for locally_linear_embedding that takes X as a NearestNeighbors object.

I have no idea about the CircleCI failure but I would just ignore this one.

@jnothman should we had a whats_new entry that says that Isomap can now fit a sparse matrix?

@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Jun 29, 2018

Member
Member

jnothman commented Jun 29, 2018

@TomDLT

TomDLT approved these changes Jun 29, 2018

Show outdated Hide outdated doc/whats_new/v0.20.rst Outdated
@lesteve

This comment has been minimized.

Show comment
Hide comment
@lesteve

lesteve Jul 2, 2018

Member

Good catch @jnothman, I just fixed it. Merging, thanks a lot @lmcinnes!

Member

lesteve commented Jul 2, 2018

Good catch @jnothman, I just fixed it. Merging, thanks a lot @lmcinnes!

@lesteve lesteve merged commit e27242a into scikit-learn:master Jul 2, 2018

0 of 5 checks passed

LGTM analysis: Python Running analyses for revisions
Details
ci/circleci: python2 CircleCI is running your tests
Details
ci/circleci: python3 CircleCI is running your tests
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment