-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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] Manifold Learning #1624
[ENH] Manifold Learning #1624
Conversation
Current coverage is 88.71% (diff: 100%)@@ master #1624 diff @@
==========================================
Files 82 82
Lines 8778 8802 +24
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
+ Hits 7783 7809 +26
+ Misses 995 993 -2
Partials 0 0
|
dfe359f
to
290a58b
Compare
self.max_iter_spin = self._create_spin_parameter( | ||
"max_iter", 10, 10 ** 4, "Max iterations:") | ||
self.random_state_check = self._create_check_parameter( | ||
"random_state", "Replicable (deterministic) learning") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Replicable (deterministic) learning" is a bit awkward and too long - does not fit into fixed widget size for me.
Suggestion: invert the option and use "Random initialization" checkbox.
Bonus question: does replicable learning use PCA initialization? because the results look different than in the MDS widget...
self.data = None | ||
|
||
# GUI | ||
info_box = gui.vBox(self.controlArea, "Info") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While the Info box at the top isn't inappropriate it is also not necessary IMHO.
And comparing this widget with similar ones: PCA, MDS, CA, k-means, hierarchical, or even any classification/regression widget - none of them have an Info box.
For t-SNE, some metric are redundant and some are hard to understand from default names given in sklearn. My proposal is: Euclidean - remove (sklearn uses Euclidean squared) With t-SNE and cosine metric, I get a complaint "All distances should be positive." The metric is 1-cos(fi), so by definition this should always yield positive. But due to numerical error, cos(fi) on iris indeed returns a negative number (-1e-16). Quick fix for this pull request: do not include cosine (@lanzagar will report an issue to sklearn). |
Fix for TSNE with cosine distances was merged in scikit-learn 2 days ago so it should work in the next version. |
No description provided.