8000 Geometric median by shubhamtalbar96 · Pull Request #1565 · geomstats/geomstats · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Geometric median #1565

New issue 8000

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

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

Jump to bottom
Merged
merged 22 commits into from
Jul 1, 2022
Merged

Geometric median #1565

merged 22 commits into from
Jul 1, 2022

Conversation

shubhamtalbar96
Copy link
Contributor

Added Geometric Median algorithm to Geomstats, contains appropriate test cases and code coverage.

@codecov
Copy link
codecov bot commented Jun 5, 2022 •
edited
Loading

Codecov Report

Merging #1565 (073659b) into master (a009881) will increase coverage by 2.86%.
The diff coverage is 97.83%.

@@            Coverage Diff             @@
##           master    #1565      +/-   ##
==========================================
+ Coverage   88.05%   90.90%   +2.86%     
==========================================
  Files         103      109       +6     
  Lines       10166    10438     +272     
==========================================
+ Hits         8951     9488     +537     
+ Misses       1215      950     -265
Flag Coverage Δ
autograd 88.63% <97.62%> (+0.58%) ⬆️
numpy 87.09% <97.62%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
geomstats/learning/geometric_median.py 97.37% <97.37%> (ø)
geomstats/_backend/autograd/__init__.py 98.50% <100.00%> (+0.05%) ⬆️
geomstats/_backend/numpy/__init__.py 98.50% <100.00%> (ø)
geomstats/learning/frechet_mean.py 95.36% <0.00%> (ø)
geomstats/_backend/numpy/autodiff.py 90.00% <0.00%> (ø)
geomstats/_backend/numpy/linalg.py 94.12% <0.00%> (ø)
geomstats/_backend/numpy/_common.py 88.89% <0.00%> (ø)
geomstats/_backend/numpy/random.py 100.00% <0.00%> (ø)
geomstats/learning/expectation_maximization.py 80.56% <0.00%> (+0.56%) ⬆️
... and 7 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 a009881...073659b. Read the comment docs.

ninamiolane
ninamiolane reviewed Jun 6, 2022
View reviewed changes
8000
Copy link
Collaborator
@ninamiolane ninamiolane left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Nice! A few minor changes needed:

  • see DeepSource GitHub action result and address them,
  • see my inline comments.

@LPereira95 see my comment about whether we should enforce using the new tests structure here.

@ninamiolane ninamiolane requested a review from luisfpereira June 6, 2022 14:01
luisfpereira
luisfpereira requested changes Jun 7, 2022
View reviewed changes
Copy link
Collaborator
@luisfpereira luisfpereira left a comment

Choose a reason for hiding this comment

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

Thanks @shubhamtalbar96!

Besides the comments I've made, can you please add tests to divide that cover the ignore_div_zero case?

@luisfpereira luisfpereira mentioned this pull request Jun 8, 2022
Closed
8 tasks
@ninamiolane
Copy link
Collaborator

@shubhamtalbar96 any update on this? Thanks! 🙌

@shubhamtalbar96
Copy link
Contributor Author
shubhamtalbar96 commented Jun 13, 2022 •
edited
Loading

@ninamiolane will get back on this shortly, apologies for the delay.

@luisfpereira luisfpereira mentioned this pull request Jul 1, 2022
Closed
@luisfpereira luisfpereira reopened this Jul 1, 2022
@luisfpereira
Copy link
Collaborator
luisfpereira commented Jul 1, 2022 •
edited
Loading

@shubhamtalbar96, I've made small changes to the code. Please check if you agree with them.

I've also added a sanity check for the manifolds whose points are not matrices, to ensure it runs.

I find the test that contains expected data too trivial (I would be happier to test it in a more convincing case for which we know the expected result), but I guess it is OK for now.

@ninamiolane from my side is ready to go.

P.S. @SaitejaUtpala you may also be interested in review the last changes.

ninamiolane
ninamiolane approved these changes Jul 1, 2022
View reviewed changes
Copy link
Collaborator
@ninamiolane ninamiolane left a comment

Choose a reason for hiding this comment

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

Excellent, thank you so much for the help @LPereira95 ! LGTM.

@ninamiolane ninamiolane merged commit 2060c5f into geomstats:master Jul 1, 2022
@qbarthelemy qbarthelemy mentioned this pull request Sep 12, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ninamiolane ninamiolane ninamiolane approved these changes

@luisfpereira luisfpereira luisfpereira approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@shubhamtalbar96 @ninamiolane @luisfpereira @SaitejaUtpala
0