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

Enable precomputed similarity matrices #87

Merged
merged 3 commits into from Jul 12, 2018

Conversation

deargle
Copy link
Collaborator

@deargle deargle commented May 21, 2018

Clustering functions such as DBSCAN accept metric='precomputed', which requires a square distance (dissimilarity) matrix. However, the mapper isn't aware when
precomputed matrices are being passed in, so when it slices according to the filter function values and hypercubes, the square-ness is un-squared.

This PR adds a kludgey way to tell the mapper that a precomputed matrix
is being passed in. When set to True, the slice will feed a square
matrix to clusterer.fit()

Clustering functions such as `DBSCAN` accept `metric='precomputed'`, which requires a square distance (similarity) matrix to be passed as, in this case, `inverse_X`. However, the mapper isn't aware when
precomputed matrices are being passed in, so when it slices according to the filter function values and hypercubes, the square-ness is un-squared.

This PR adds a kludgey way to tell the mapper that a precomputed matrix
is being passed in. When set to `True`, the slice will feed a square
matrix to `clusterer.fit()`
@pep8speaks
Copy link
Contributor

pep8speaks commented May 21, 2018

Hello @deargle, Thank you for updating !

Line 57:242: W291 trailing whitespace
Line 59:25: W291 trailing whitespace
Line 119:17: E722 do not use bare except'
Line 123:9: E722 do not use bare except'
Line 283:59: W291 trailing whitespace
Line 554:13: E265 block comment should start with '# '

Comment last updated on May 21, 2018 at 21:02 Hours UTC

@codecov-io
Copy link

codecov-io commented May 21, 2018

Codecov Report

Merging #87 into master will decrease coverage by 0.31%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #87      +/-   ##
==========================================
- Coverage   82.15%   81.84%   -0.32%     
==========================================
  Files           6        6              
  Lines         409      413       +4     
  Branches       95       96       +1     
==========================================
+ Hits          336      338       +2     
- Misses         51       52       +1     
- Partials       22       23       +1
Impacted Files Coverage Δ
kmapper/kmapper.py 75.83% <66.66%> (-0.44%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f53a467...e15a185. Read the comment docs.

@sauln
Copy link
Member

sauln commented May 24, 2018

Hey! Thanks for this PR.

There were some variable name changes. Could you please resolve the conflicts and add a couple of unit tests showing that it works and is equivalent to the typical method?

@sauln sauln merged commit e15a185 into scikit-tda:master Jul 12, 2018
sauln pushed a commit that referenced this pull request Jul 12, 2018
Incorporate bounds into covering class, satisfying Issue #97. This also incorporates commits from PR #87.
@deargle
Copy link
Collaborator Author

deargle commented Jul 13, 2018 via email

@sauln
Copy link
Member

sauln commented Jul 13, 2018

Great, Thanks!

Please notice some of the variable names have changed!

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.

None yet

4 participants