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

Transition of DARPA SD2 Implementation of PersistenceImages() into persim module #45

Merged
merged 14 commits into from Jan 9, 2021

Conversation

francismotta
Copy link

This fork contains a large update to the persistence images module through the implementation of the PersistenceImages() class, which developed as part of the DARPA Synergistic Discovery and Design program. New functionality includes:

  • Flexible image parameter setting and enhanced .fit() and .fit_transform methods
  • Optimized performance for Gaussian kernels
  • Efficient calculation using summed-area tables
  • Support for custom kernel functions and implementations of general Gaussians and uniform kernels
  • Support for custom weighting functions and implementations of linear ramp and persistence weights
  • Optional parallelization over collections of diagrams
  • New plotting routines for diagrams and images
  • Additional unit tests
  • New and updated documentation and example notebooks

Francis Motta added 4 commits October 12, 2020 16:36
…tation inside the class, moved transform() method to _transform() and madea new transform() method which ingests one or more persistence diagrams
…unction, updated usage comments in PersistenceImages(), wrote uniform kernel function, updated tutorials in jupyter notebook
…hed several bugs included warnings related to using np.copy() in PersistenveImager().fit_transform(), added uniform() to images.py for use as a kernel function, added example of updating weight function to Persistence Images.ipynb
…ed Classificaiton with persistence images notebook to use new PersistenceImages() class, added paralellization example to Persistence Image notebook, made all unit tests use numpt.testing assert methods
@codecov-io
Copy link

codecov-io commented Dec 17, 2020

Codecov Report

Merging #45 (3538703) into master (76b2b5a) will decrease coverage by 6.13%.
The diff coverage is 64.26%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #45      +/-   ##
==========================================
- Coverage   80.90%   74.76%   -6.14%     
==========================================
  Files          10       12       +2     
  Lines         550      868     +318     
  Branches      114      160      +46     
==========================================
+ Hits          445      649     +204     
- Misses         77      177     +100     
- Partials       28       42      +14     
Impacted Files Coverage Δ
persim/images_kernels.py 51.57% <51.57%> (ø)
persim/images.py 71.37% <66.99%> (-10.98%) ⬇️
persim/images_weights.py 100.00% <100.00%> (ø)

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 76b2b5a...3538703. Read the comment docs.

persim/images.py Outdated Show resolved Hide resolved
persim/images.py Outdated Show resolved Hide resolved
persim/images.py Outdated Show resolved Hide resolved
persim/images.py Outdated Show resolved Hide resolved
@sauln
Copy link
Member

sauln commented Dec 17, 2020

This is great! Thank you 💯

I gave the code a very brief skim and added a few comments. @ctralie do you have bandwidth to give a more thorough review of the implementation details?

@ctralie
Copy link
Member

ctralie commented Dec 18, 2020 via email

- Modified PersistenceImager().__repr__ to create a valid constructor.
- Removed PersistenceImager().dict_print and replaced with prettyprint.
- Changed default kernel function name from bvncdf to gaussian to make more understandable.
- Added a new test to ensure guassian parameter sigma can be either a numpy array or a list of lists.
- Renamed some kernel functions by removing leading underscore to ensure proper loading.
- Updated block comments in PersistenceImager() to reflect updated __repr__ method.
test/test_persim_update.py Outdated Show resolved Hide resolved
@sauln
Copy link
Member

sauln commented Dec 28, 2020

This looks great by the way! 💯 The final thing is adding some bits to the documentation.

@sauln
Copy link
Member

sauln commented Dec 28, 2020

@francismotta For the docs, you'll have to add an entry here and make sure that everything renders correctly. It should be pretty straight forward to build. If I remember correctly, just running a make html in the docs directory and then opening the generated build/index.html should let you poke around what was buit.

…eights.py

- Made plot methods in PersistenceImager return Axes object
- Deprecated PersImage class using deprecation sphinx decorator
@francismotta
Copy link
Author

francismotta commented Dec 29, 2020

@sauln Documentation updated and renders nicely! I also deprecated PersImage. By the way, make html produces a bunch of warnings unrelated to PersistenceImager:

WARNING: toctree contains reference to document 'notebooks/distances' that doesn't have a title: no link will be generated

There seems to be something wrong with this notebook (in my fork and the published version). I am unable to open it:

Unreadable Notebook: D:\research\sd2\work\persim\docs\notebooks\distances.ipynb NotJSONError('Notebook does not appear to be JSON: \'{\\n "cells": [\\n {\\n "cell_type": "m...',)

Francis Motta added 2 commits December 30, 2020 11:55
… valid string, updated documentation, added parameter validation method and unittests
persim/images.py Show resolved Hide resolved
@sauln
Copy link
Member

sauln commented Jan 5, 2021

I was able to fix the problems with the distances.ipynb notebook. If you merge master into your fork, it should be working for you now.

Francis Motta added 5 commits January 5, 2021 17:41
…x autodoc warning related to use of |V(G)|x|V(G)|
…ted class methods and attributes, changed member order to groupwise from its default alphabetical to ensure class methods appear grouped together in the documentation html ahead of attributes which alspo appear grouped together, added docstrings for public attributes of PersistenceImager
@sauln sauln merged commit a8207db into scikit-tda:master Jan 9, 2021
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