Skip to content

Conversation

@garrettwrong
Copy link
Collaborator

@garrettwrong garrettwrong commented May 20, 2021

Adds contributed code for wavelet based earth mover distance approximation.

Currently I stubbed in a smoke test, but Amit M to work on a more suitable test. I don't think that we need to exercise the theoretical bounds in the test. Maybe we can just see if distance between two controlled (gaussian?) clouds is reasonable.

While I admittedly liked the syntax of the docstring a little more in the original contribution, it was changed to be more consistent with most of the project.

@garrettwrong garrettwrong added the enhancement New feature or request label May 20, 2021
@codecov
Copy link

codecov bot commented May 20, 2021

Codecov Report

Merging #419 (5d4ac32) into develop (004ba5e) will increase coverage by 0.97%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #419      +/-   ##
===========================================
+ Coverage    86.93%   87.91%   +0.97%     
===========================================
  Files           91       96       +5     
  Lines         6171     6851     +680     
===========================================
+ Hits          5365     6023     +658     
- Misses         806      828      +22     
Impacted Files Coverage Δ
src/aspire/operators/__init__.py 100.00% <100.00%> (ø)
src/aspire/operators/wemd.py 100.00% <100.00%> (ø)
src/aspire/commands/estimate_ctf.py 0.00% <0.00%> (ø)
src/aspire/ctf/ctf_estimator.py 98.87% <0.00%> (ø)
src/aspire/commands/_config.py 0.00% <0.00%> (ø)
src/aspire/ctf/__init__.py 100.00% <0.00%> (ø)
src/aspire/covariance/covar2d.py 93.59% <0.00%> (+1.04%) ⬆️
src/aspire/utils/misc.py 98.38% <0.00%> (+1.16%) ⬆️
... and 2 more

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 004ba5e...5d4ac32. Read the comment docs.

@garrettwrong garrettwrong linked an issue May 20, 2021 that may be closed by this pull request
@garrettwrong
Copy link
Collaborator Author

Closes #418

@garrettwrong
Copy link
Collaborator Author

@mosco , any word on the unit test progress? Thanks

@mosco mosco marked this pull request as ready for review June 7, 2021 19:49
@mosco mosco requested a review from janden as a code owner June 7, 2021 19:49
@mosco
Copy link
Contributor

mosco commented Jun 7, 2021

@mosco , any word on the unit test progress? Thanks

Unit test is done. I also updated the code slightly to support general n-dimensional arrays and non-normalized arrays.

@garrettwrong
Copy link
Collaborator Author

Outstanding, thanks! I was getting worried 😅.

I looked it over and fixed up the syntax checking, also some line length tweaks that would have probably come up in review.

If the test passes on all the platforms it should be ready for Joakim to review.

Thanks!

Copy link
Collaborator

@janden janden left a comment

Choose a reason for hiding this comment

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

Looks good. Just a few things.

Copy link
Collaborator

@janden janden left a comment

Choose a reason for hiding this comment

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

Just one more thing.

@garrettwrong garrettwrong requested a review from janden June 8, 2021 16:13
@garrettwrong garrettwrong merged commit c2b89e2 into develop Jun 8, 2021
@garrettwrong garrettwrong deleted the add_wbemd_418 branch June 8, 2021 16:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add Wavelet Based Earth Mover Distance

4 participants