8000 Make default_point_type a property by luisfpereira · Pull Request #1644 · geomstats/geomstats · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Make default_point_type a property #1644

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.

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 6 commits into from
Sep 21, 2022
Merged

Make default_point_type a property #1644

merged 6 commits into from
Sep 21, 2022

Conversation

luisfpereira
Copy link
Collaborator

This PR makes default_point_dtype a property (computed from shape).

shape has been corrected for several spaces and/or metrics and a general test to validate shape against the shape of a generated random point was added.

@codecov
Copy link
codecov bot commented Sep 16, 2022 •
edited
Loading

Codecov Report

Merging #1644 (46e3a57) into master (c2d73cb) will increase coverage by 3.97%.
The diff coverage is 88.80%.

@@            Coverage Diff             @@
##           master    #1644      +/-   ##
==========================================
+ Coverage   88.19%   92.16%   +3.97%     
==========================================
  Files         115      121       +6     
  Lines       11523    11782     +259     
==========================================
+ Hits        10162    10858     +696     
+ Misses       1361      924     -437
Flag Coverage Δ
autograd 87.49% <88.80%> (-0.01%) ⬇️
numpy 88.22% <88.80%> (?)
tensorflow 74.33% <87.07%> (-0.06%) ⬇️

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

Impacted Files Coverage Δ
geomstats/geometry/base.py 94.85% <ø> (-0.10%) ⬇️
geomstats/geometry/euclidean.py 94.12% <ø> (ø)
geomstats/geometry/lower_triangular_matrices.py 100.00% <ø> (ø)
geomstats/geometry/matrices.py 97.24% <ø> (-0.01%) ⬇️
geomstats/geometry/pre_shape.py 92.49% <ø> (ø)
geomstats/geometry/rank_k_psd_matrices.py 98.04% <ø> (ø)
geomstats/geometry/sasaki_metric.py 98.94% <ø> (ø)
geomstats/geometry/spd_matrices.py 95.72% <ø> (ø)
geomstats/geometry/stiefel.py 79.67% <ø> (ø)
geomstats/geometry/symmetric_matrices.py 92.05% <ø> (+1.14%) ⬆️
... and 47 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@luisfpereira
Copy link
Collaborator Author

@ninamiolane failures are unrelated with this PR. In fact, the failure in the example is related with last release of matplotlib (to be address in another PR).

@luisfpereira luisfpereira mentioned this pull request Sep 16, 2022
Merged
ninamiolane
ninamiolane requested changes Sep 16, 2022
View reviewed changes
ninamiolane
ninamiolane reviewed Sep 16, 2022
View reviewed changes
@ninamiolane
Copy link
Collaborator

Great, thank you! Why not removing default_point_type and point_type completely? (instead of making it a property?)

It could be simpler to have only one variable ("shape") referring to that one concept.

@luisfpereira
Copy link
Collaborator Author

I think it may be valuable to keep default_point_type, because its current use case is the bool self.default_point_type == "vector" or self.default_point_type == "matrix", which in my opinion is more readable than len(self.shape) == 1 or len(self.shape)>1. But if you think it is better to use len(self.shape), it is completely fine to me.( Anyway, default_point_type is never assigned, except for the product manifold case.)

Regarding default_point_type/point_type and default_coords_type/coords_type, I think it needs homogenization (remove default?). I guess this naming exists due to some history and the distinction is not worthy anymore.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

@luisfpereira luisfpereira merged commit d7764cf into geomstats:master Sep 21, 2022
@luisfpereira luisfpereira deleted the default_point_type branch October 6, 2022 15:18
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

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

Successfully merging this pull request may close these issues.
2 participants
@luisfpereira @ninamiolane
0