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] Manifold Learning #1624

Merged
merged 2 commits into from
Oct 28, 2016
Merged

[ENH] Manifold Learning #1624

merged 2 commits into from
Oct 28, 2016

Conversation

VesnaT
Copy link
Contributor

@VesnaT VesnaT commented Oct 3, 2016

No description provided.

@codecov-io
Copy link

codecov-io commented Oct 3, 2016

Current coverage is 88.71% (diff: 100%)

Merging #1624 into master will increase coverage by 0.05%

@@             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          

Sunburst

Powered by Codecov. Last update 340a25d...bc9ff5f

@VesnaT VesnaT force-pushed the manifold branch 3 times, most recently from dfe359f to 290a58b Compare October 3, 2016 14:51
@VesnaT VesnaT changed the title Manifold Learning [ENH] Manifold Learning Oct 6, 2016
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")
Copy link
Contributor

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")
Copy link
Contributor

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.

@BlazZupan
Copy link
Contributor

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)
L1 - remove (same as Manhattan)
L2 - keep, but rename to Euclidean
Manhattan - keep
Cosine - keep, but check what is wrong
Chebyshev - keep
Jaccard - keep
Mahalanobis - keep
Minkowski - remove (would need a parameter p, if it stays, but we actually do not need it here)
Seuclidean - remove (needs parameter)
Sqeuclidean - remove

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).

@VesnaT VesnaT changed the title [ENH] Manifold Learning [WIP][ENH] Manifold Learning Oct 24, 2016
@VesnaT VesnaT changed the title [WIP][ENH] Manifold Learning [ENH] Manifold Learning Oct 25, 2016
@lanzagar
Copy link
Contributor

Fix for TSNE with cosine distances was merged in scikit-learn 2 days ago so it should work in the next version.
scikit-learn/scikit-learn#7732

@BlazZupan BlazZupan merged commit 1cb84b2 into biolab:master Oct 28, 2016
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.

4 participants