8000 Use `p` for the rank of Grassmannian manifolds by p16i · Pull Request #1586 · geomstats/geomstats · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Use p for the rank of Grassmannian manifolds #1586

New issue < 8000 button aria-label="Close dialog" data-close-dialog="" type="button" data-view-component="true" class="Link--muted btn-link position-absolute p-4 right-0">

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 2 commits into from
Jul 1, 2022
Merged

Use p for the rank of Grassmannian manifolds #1586

merged 2 commits into from
Jul 1, 2022

Conversation

p16i
Copy link
Contributor
@p16i p16i commented Jun 18, 2022

Checklist

  • My pull request has a clear and explanatory title.
  • If neccessary, my code is vectorized.
  • I have added apropriate unit tests.
  • I have made sure the code passes all unit tests. (refer to comment below)
  • My PR follows PEP8 guidelines. (refer to comment below)
  • My PR follows geomstats coding style and API.
  • My code is properly documented and I made sure the documentation renders properly. (Link)
  • I have linked to issues and PRs that are relevant to this PR.

Description

The goal of this PR is to use a consistent variable in grassmannian.py. Currently, the rank referred in the module and its document is either p or k.

Issue

#1557

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

Codecov Report

Merging #1586 (0ba74cb) into master (399d33e) will increase coverage by 0.49%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1586      +/-   ##
==========================================
+ Coverage   90.39%   90.87%   +0.49%     
==========================================
  Files         108      108              
  Lines       10364    10392      +28     
==========================================
+ Hits         9367     9443      +76     
+ Misses        997      949      -48
Flag Coverage Δ
autograd 88.59% <100.00%> (+0.53%) ⬆️
numpy 87.06% <100.00%> (+0.51%) ⬆️

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

Impacted Files Coverage Δ
geomstats/geometry/grassmannian.py 93.69% <100.00%> (ø)
geomstats/learning/kmedoids.py 92.60% <0.00%> (-3.70%) ⬇️
geomstats/geometry/product_riemannian_metric.py 90.00% <0.00%> (-1.40%) ⬇️
geomstats/learning/frechet_mean.py 95.36% <0.00%> (ø)
geomstats/geometry/product_manifold.py 97.15% <0.00%> (+0.03%) ⬆️
geomstats/learning/expectation_maximization.py 80.56% <0.00%> (+0.56%) ⬆️
geomstats/geometry/euclidean.py 97.06% <0.00%> (+2.95%) ⬆️
geomstats/geometry/discrete_curves.py 88.60% <0.00%> (+9.61%) ⬆️
geomstats/geometry/landmarks.py 100.00% <0.00%> (+11.12%) ⬆️

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 399d33e...0ba74cb. Read the comment docs.

@luisfpereira luisfpereira self-requested a review July 1, 2022 07:48
luisfpereira
luisfpereira approved these changes Jul 1, 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.

Looks ready from my side @ninamiolane.

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.

LGTM, many thanks!

@ninamiolane ninamiolane merged commit 3cd0010 into geomstats:master Jul 1, 2022
@luisfpereira luisfpereira mentioned this pull request Jul 11, 2023
Closed
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.
3 participants
@p16i @ninamiolane @luisfpereira
0