8000 API Add `"auto"` value and deprecate default value for `normalized_stress` by Micky774 · Pull Request #23834 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

API Add "auto" value and deprecate default value for normalized_stress #23834

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
Merged
merged 17 commits into from
Oct 10, 2022
Merged

API Add "auto" value and deprecate default value for normalized_stress #23834

merged 17 commits into from
Oct 10, 2022

Conversation

Micky774
Copy link
Contributor
@Micky774 Micky774 commented Jul 4, 2022

Reference Issues/PRs

Follow-up to #22562

What does this implement/fix? Explain your changes.

Adds "auto" option to normalized_stress in MDS and _mds.smacof, which will enable normalized_stress if metric=False, and disable otherwise. Began deprecation to change default value to "auto".

Any other comments?

Next step after this PR is to deprecate normalized_stress all-together and only adopt the "auto" semantics.

thomasjpfan
thomasjpfan reviewed Jul 4, 2022
View reviewed changes
Copy link
Member
@thomasjpfan thomasjpfan left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Next step after this PR is to deprecate normalized_stress all-together and only adopt the "auto" semantics.

This would disable a configuration that works today, i.e. normalized_stress=False and metric=False. Is there a use case for this configuration?

@Micky774
Copy link
Contributor Author
Micky774 commented Jul 4, 2022 •
edited
Loading

This would disable a configuration that works today. normalized_stress, i.e. normalized_stress=False and metric=False. Is there a use case for this configuration?

From what I've read so far, not really. It seems that for non-metric MDS, normalized stress (Stress-1) is almost alays used. This is consistent w/ the R implementations of non-metric MDS. I'll do some more review on the matter (was planning on so after this PR) but as of right now everything I've seen is pretty one-sided suggesting that unnormalized stress is strictly worse than normalized stress in the non-metric case.

@Micky774
Copy link
Contributor Author
Micky774 commented Jul 6, 2022

IIndeed, the R implementation only exposes Stress-1, not the raw stress. I think it is fine to not support raw stress for non-metric MDS. Furthermore, Kruscal's initial numerical implementation was explicitly optimizing Stress-1 as opposed to raw stress. With these I'm pretty confident we can safely abandon raw stress for non-metric MDS.

thomasjpfan
thomasjpfan reviewed Jul 19, 2022
View reviewed changes
Copy link
Member
@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

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

When normalized_stress="auto", the eps would be a bit harder to tune because of the automatic switching between normalized stress and un-normalized stress.

Since literature and the R implementation only uses the normalized stress for non-metric MDS, I am overall okay with this PR.

thomasjpfan
thomasjpfan reviewed Jul 19, 2022
View reviewed changes
@thomasjpfan thomasjpfan changed the title MNT: Add "auto" value and deprecate default value for normalized_stress API Add "auto" value and deprecate default value for normalized_stress Jul 19, 2022
thomasjpfan
thomasjpfan approved these changes Jul 27, 2022
View reviewed changes
Copy link
Member
@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

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

I'm overall happy with this change. LGTM

@jeremiedbb
Copy link
Member

Since normalized_stress param has not been released yet, we can modifiy its default value without deprecation

@jeremiedbb jeremiedbb added this to the 1.2 milestone Sep 22, 2022
@thomasjpfan
Copy link
Member

Even tho normalized_stress is a new parameter, the default normalized_stress=False is the current behavior on main. This PR wants to change the default to "auto", which sets normalized_stress to not metric. We can not change it to "auto" right away because it would be backward incompatible.

@jeremiedbb
Copy link
Member

Even tho normalized_stress is a new parameter, the default normalized_stress=False is the current behavior on main. This PR wants to change the default to "auto", which sets normalized_stress to not metric. We can not change it to "auto" right away because it would be backward incompatible.

Right, I read too quickly

jeremiedbb
jeremiedbb approved these changes Oct 7, 2022
View reviewed changes
Copy link
Member
@jeremiedbb jeremiedbb left a comment

Choose a reason for hiding this comment

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

LGTM

thomasjpfan
thomasjpfan reviewed Oct 9, 2022
View reviewed changes
@Micky774 @thomasjpfan
Apply suggestions from code review
6136c88 
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
@thomasjpfan thomasjpfan merged commit b0b7c15 into scikit-learn:main Oct 10, 2022
@Micky774 Micky774 deleted the mds_normalized_auto branch October 10, 2022 18:45
glemaitre pushed a commit to glemaitre/scikit-learn that referenced this pull request Oct 31, 2022
@Micky774 @thomasjpfan @glemaitre
API Add "auto" value and deprecate default value for `normalized_st…
17f12a3 
…ress` (scikit-learn#23834)

Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thomasjpfan thomasjpfan thomasjpfan approved these changes

@jeremiedbb jeremiedbb jeremiedbb approved these changes

Assignees
No one assigned
Labels
module:manifold Waiting for Reviewer
Projects
None yet
Milestone
1.2
Development

Successfully merging this pull request may close these issues.
3 participants
@Micky774 @jeremiedbb @thomasjpfan
0