Skip to content
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

[ENH] MDS: Show Kruskal stress #6309

Merged
merged 1 commit into from
Feb 17, 2023
Merged

Conversation

janezd
Copy link
Contributor

@janezd janezd commented Jan 23, 2023

Issue

Fixes #6070.

Description of changes

To reviewer: please check that I indeed called update_stress from all places where it's needed -- and that it's not called multiple times.

I don't know where to show the stress. Optimization box was about the best place. I don't want to put it on the graph, because then we'd need to at least allow the user to move it and ... no.

Includes
  • Code changes
  • Tests
  • Documentation

@janezd janezd force-pushed the mds-kruskal-stress branch from ff64af7 to fed44cb Compare January 23, 2023 18:53
@codecov
Copy link

codecov bot commented Jan 23, 2023

Codecov Report

Merging #6309 (0d145ab) into master (a7d215e) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #6309   +/-   ##
=======================================
  Coverage   87.47%   87.48%           
=======================================
  Files         316      316           
  Lines       68085    68115   +30     
=======================================
+ Hits        59556    59589   +33     
+ Misses       8529     8526    -3     

@janezd janezd force-pushed the mds-kruskal-stress branch from fed44cb to cbfedc3 Compare January 24, 2023 09:48
@lanzagar lanzagar changed the title MDS: Show Kruskal stress [ENH] MDS: Show Kruskal stress Feb 2, 2023
@@ -200,6 +200,7 @@ def __init__(self):

self.embedding = None # type: Optional[np.ndarray]
self.effective_matrix = None # type: Optional[DistMatrix]
self.stress = None
Copy link
Contributor

Choose a reason for hiding this comment

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

As shown by failing tests, this conflicts with the static method with the same name.
This results in a crash when setting the size of points to "Stress".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not that I don't warn students about this exact mistake.

@janezd janezd self-assigned this Feb 3, 2023
@janezd janezd force-pushed the mds-kruskal-stress branch from cbfedc3 to 56eff89 Compare February 3, 2023 08:46
@janezd janezd removed their assignment Feb 3, 2023
@janezd janezd force-pushed the mds-kruskal-stress branch from 56eff89 to 2e01854 Compare February 3, 2023 08:53
Comment on lines 416 to 421
def _compute_stress(self):
if self.embedding is None or self.effective_matrix is None:
return None
actual = scipy.spatial.distance.pdist(self.embedding)
actual = scipy.spatial.distance.squareform(actual)
return np.sqrt(np.sum((actual - self.effective_matrix) ** 2)
/ (np.sum(self.effective_matrix ** 2) or 1))
Copy link
Contributor

Choose a reason for hiding this comment

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

It would probably be better to call get_stress and normalize the returned result.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@janezd janezd force-pushed the mds-kruskal-stress branch from 2e01854 to 0d145ab Compare February 11, 2023 15:45
@lanzagar lanzagar merged commit c42459a into biolab:master Feb 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MDS: amount of explained variance
2 participants