8000 [MRG] Issue 1453: MDS fall back to SVD when possible by NelleV · Pull Request #4485 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

[MRG] Issue 1453: MDS fall back to SVD when possible #4485

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 3 commits into from
Closed

[MRG] Issue 1453: MDS fall back to SVD when possible #4485

wants to merge 3 commits into from

Conversation

NelleV
Copy link
Member
@NelleV NelleV commented Apr 2, 2015

This is a follow up on PR #3141.

I've fixed most of @ogrisel's comment.

@NelleV NelleV mentioned this pull request Apr 2, 2015
Closed
@landscape-bot
Copy link

Code Health
Repository health increased by 0.00% when pulling 2529eb3 on NelleV:mds into 6e54079 on scikit-learn:master.

@landscape-bot
Copy link

Code Health
Repository health increased by 0.00% when pulling a7cd7cb on NelleV:mds into 6e54079 on scikit-learn:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 95.13% when pulling a7cd7cb on NelleV:mds into 6e54079 on scikit-learn:master.

@landscape-bot
Copy link

Code Health
Repository health increased by 0.00% when pulling 8f59b23 on NelleV:mds into 6e54079 on scikit-learn:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 95.13% when pulling 8f59b23 on NelleV:mds into 6e54079 on scikit-learn:master.

@NelleV
Minor changes in the classical mds
bf04a74 
- check_arrays was deprecated. Changed to check_array
- eigenvalue is now done using scipy.sparse.linalg, thus allowing to retreive
  only the k largest eigenvalues and vectors
- uses sklearn.metrics.euclidean_distances when possible
@NelleV
Copy link
Member Author
NelleV commented Apr 2, 2015

@GaelVaroquaux @ogrisel I think this is ready.

NelleV
NelleV reviewed Apr 2, 2015
View reviewed changes
sklearn/manifold/mds.py
@@ -280,6 +320,10 @@ class MDS(BaseEstimator):
compute metric or nonmetric SMACOF (Scaling by Majorizing a
Complicated Function) algorithm

method: string, optional, default: smacof
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may want to switch the default to svd: it solves the exact problem, and should be much faster.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. Can you do a quick benchmark?

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 95.13% when pulling bf04a74 on NelleV:mds into 1c33a6f on scikit-learn:master.

@NelleV NelleV changed the title Issue 1453: MDS fall back to SVD when possible [MRG] Issue 1453: MDS fall back to SVD when possible Apr 2, 2015
amueller
amueller reviewed Apr 2, 2015
View reviewed changes
sklearn/manifold/mds.py
@@ -106,7 +107,9 @@ def _smacof_single(similarities, metric=True, n_components=2, init=None,
(disparities ** 2).sum())

# Compute stress
stress = ((dis.ravel() - disparities.ravel()) ** 2).sum() / 2
stress = (euclidean_distances(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is a squared parameter to euclidean_distances.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you change that, by the way?

@webdrone
Copy link

Hello,

I'm sort of picking up the various threads of this MDS problem extending back into the years... so apologies for the many comments everywhere.

Is there any plan of merging this so that I can add a transform method to the SVD version, which is amenable to it? If you merge it I will be able to change #6222 in accordance to this rather than re-engineering with an extensible option.

Edit: the eigen-decomposition could also be using linalg.eigsh since the matrix will be PSD and symmetric.

@webdrone webdrone mentioned this pull request Jan 26, 2016
Closed
@yxtay yxtay mentioned this pull request Jun 18, 2016
Closed
@amueller
Copy link
Member
amueller commented Oct 7, 2016

@NelleV given your recent activity, do you want to pick up this one?

@jnothman jnothman added this to the 0.20 milestone Sep 15, 2017
@jsoutherland jsoutherland mentioned this pull request Sep 25, 2017
Open
@amueller
Copy link
Member

moving to 0.21

@amueller amueller modified the milestones: 0.20, 0.21 Jul 16, 2018
@jnothman
Copy link
Member

Would be nice if someone could pick this up.

@jnothman jnothman modified the milestones: 0.21, 0.22 Apr 16, 2019
@amueller amueller mentioned this pull request Oct 17, 2019
Open
@amueller amueller modified the milestones: 0.22, 0.23 Oct 17, 2019
@adrinjalali
Copy link
Member

removing from the milestone.

@adrinjalali adrinjalali removed this from the 0.23 milestone Apr 21, 2020
Base automatically changed from master to main January 22, 2021 10:48
@thomasjpfan thomasjpfan added the Superseded PR has been replace by a newer PR label Jul 21, 2022
@thomasjpfan
Copy link
Member

I am closing this PR, because it is superseded by #22330. Thank you for working on this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
help wanted module:manifold Stalled Superseded PR has been replace by a newer PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
0