Skip to content

[MRG] Feature: calculate normed stress (Stress-1) in sklearn.manifold.MDS#13042

Closed
matthieu-pa wants to merge 12 commits intoscikit-learn:mainfrom
matthieu-pa:stress1
Closed

[MRG] Feature: calculate normed stress (Stress-1) in sklearn.manifold.MDS#13042
matthieu-pa wants to merge 12 commits intoscikit-learn:mainfrom
matthieu-pa:stress1

Conversation

@matthieu-pa
Copy link
Copy Markdown

@matthieu-pa matthieu-pa commented Jan 24, 2019

Reference Issues/PRs

Fixes #10168 #12285

What does this implement/fix? Explain your changes.

This is a follow-up on the stale PRs referenced above, the main diff is the fix for the previously failing unit test:

https://travis-ci.org/scikit-learn/scikit-learn/jobs/437566342#L2818

        stress1 = mds.smacof(sim, normalize=True)[1]
        stress2 = mds.smacof(k * sim, normalize=True)[1]
    
        # Normed stress should be the same for
        # values multiplied by some factor "k"
>       assert_allclose(stress1, stress2)
k          = 2
sim        = array([[0, 5, 3, 4],
       [5, 0, 2, 2],
       [3, 2, 0, 1],
       [4, 2, 1, 0]])
stress1    = 0.025998852705994606
stress2    = 0.033375197002665315
/home/travis/build/scikit-learn/scikit-learn/sklearn/manifold/tests/test_mds.py:78: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
/usr/lib/python2.7/dist-packages/numpy/testing/utils.py:1183: in assert_allclose
    verbose=verbose, header=header)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

To my understanding, even using normalized stress, smacof() needs to be initialized at same configuration for the property Normed stress should be the same for values multiplied by some factor "k" to be true so I set random_state of smacof() to a fixed value. Dissimilarity matrix also needs to be large enough.

Any other comments?

The previous reviewer was @glemaitre . To my understanding review comments have been addressed but if something is missing, I'll do my best to fix it.

@amueller amueller added the Needs Benchmarks A tag for the issues and PRs which require some benchmarks label Aug 5, 2019
@ADEscobar
Copy link
Copy Markdown

ADEscobar commented Apr 19, 2020

Is there anything still missing for the merge?
The Stress-1 feature is actually quite fundamental to understand if the fit is meaningless or not.

@matthieu-pa
Copy link
Copy Markdown
Author

matthieu-pa commented Apr 20, 2020 via email

@matthieu-pa
Copy link
Copy Markdown
Author

matthieu-pa commented Apr 21, 2020

I merged the latest master version into this branch and solved the merge conflict.

@ADEscobar
Copy link
Copy Markdown

ADEscobar commented Apr 21, 2020 •
edited
Loading

I merged the latest master version into this branch and solved the merge conflict.

Is it required to compute the Stress-1 in every iteration, or can it be just done for the final (returned) Stress value?

Maybe it can be just a new returned value, instead of a new option. Keeping stress and adding stress_one or stress_normalized

@jnothman
Copy link
Copy Markdown
Member

jnothman commented Apr 22, 2020

Is it required to compute the Stress-1 in every iteration, or can it be just done for the final (returned) Stress value?

This is a very good question. Does norming in every iteration affect the result too?

@ADEscobar
Copy link
Copy Markdown

ADEscobar commented Apr 22, 2020

Is it required to compute the Stress-1 in every iteration, or can it be just done for the final (returned) Stress value?

This is a very good question. Does norming in every iteration affect the result too?

The stop condition (eps) is checked using the normalized stress, so it might stop prematurely and perform less iterations, since the epsilon in the normalized stress is comparatively smaller.

Not a big deal, one could just decrease the eps if using the normalized option, but I think it can anyway be more efficient doing the normalization just at the end.

@matthieu-pa
Copy link
Copy Markdown
Author

matthieu-pa commented Apr 22, 2020

Not a big deal, one could just decrease the eps if using the normalized option, but I think it can anyway be more efficient doing the normalization just at the end.

Thank you for raising a very good point. I also agree that checking the normalized stress at every iteration is not very likely to cause MDS to stop early whereas it is quite more computing intensive.

Next week, to be thorough, I could benchmark a version calculating the normalized stress at every iteration and one just at the end over a few randomly generated distance matrices. I would mainly focus on comparing execution time and the number of iterations required to converge.

@joshuacwnewton joshuacwnewton mentioned this pull request Aug 7, 2020
Closed
Base automatically changed from master to main January 22, 2021 10:50
@cmarmo cmarmo added Superseded PR has been replace by a newer PR and removed Waiting for Reviewer labels Mar 25, 2021
@Micky774 Micky774 mentioned this pull request Feb 20, 2022
Merged
@thomasjpfan thomasjpfan mentioned this pull request Apr 27, 2022
Merged
@cmarmo
Copy link
Copy Markdown
Contributor

cmarmo commented May 2, 2022

Closing as superseded by #22562.

@cmarmo cmarmo closed this May 2, 2022
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

module:manifold Needs Benchmarks A tag for the issues and PRs which require some benchmarks 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.

6 participants

@matthieu-pa @ADEscobar @jnothman @cmarmo @amueller @mattmilten