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

changes to SEmanifold visualization #1812

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

changes to SEmanifold visualization #1812

wants to merge 6 commits into from

Conversation

mnamazi1
Copy link
Collaborator

@mnamazi1 mnamazi1 commented Jan 30, 2023

Checklist

  • My pull request has a clear and explanatory title.
  • If necessary, my code is vectorized.
  • I added appropriate unit tests.
  • I 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 linked to issues and PRs that are relevant to this PR.

Description

Changes to SE(2) and addition of SE(3) manifold visualization methods

Issue

Additional context

@monsij monsij mentioned this pull request Jan 30, 2023
8 tasks
@codecov
Copy link

codecov bot commented Jan 30, 2023

Codecov Report

Merging #1812 (4c8b129) into master (6f37453) will decrease coverage by 1.27%.
The diff coverage is 74.08%.

@@            Coverage Diff             @@
##           master    #1812      +/-   ##
==========================================
- Coverage   91.50%   90.22%   -1.27%     
==========================================
  Files         136      130       -6     
  Lines       13738    13372     -366     
==========================================
- Hits        12569    12063     -506     
- Misses       1169     1309     +140     
Flag Coverage Δ
autograd ?
numpy 87.10% <74.08%> (-<0.01%) ⬇️
pytorch 80.53% <74.08%> (+0.03%) ⬆️

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

Impacted Files Coverage Δ
geomstats/visualization/special_euclidean.py 82.42% <74.08%> (-9.89%) ⬇️
geomstats/information_geometry/gamma.py 57.47% <0.00%> (-33.96%) ⬇️
geomstats/_backend/_common.py 75.00% <0.00%> (-25.00%) ⬇️
geomstats/geometry/hypersphere.py 81.33% <0.00%> (-6.96%) ⬇️
geomstats/learning/geodesic_regression.py 79.88% <0.00%> (-3.89%) ⬇️
geomstats/geometry/positive_reals.py 93.48% <0.00%> (-2.17%) ⬇️
geomstats/geometry/discrete_curves.py 91.38% <0.00%> (-1.32%) ⬇️
geomstats/geometry/pullback_metric.py 90.61% <0.00%> (-1.10%) ⬇️
geomstats/geometry/sasaki_metric.py 97.88% <0.00%> (-1.06%) ⬇️
geomstats/geometry/sub_riemannian_metric.py 93.14% <0.00%> (-0.98%) ⬇️
... and 12 more

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

@mnamazi1
Copy link
Collaborator Author

mnamazi1 commented Jan 31, 2023

Task Distribution:
Monsij Biswal - ported plot_geodesic for SE(2), test cases for SE(2), minor fixes, merged all code and made consistent across classes, debugging linting
Mahmoud Namazi - created working SE(3) class, ported plot_geodesic for SE(3), test cases for SE(3), adapted geodesic function for input arguments, debugging linting
Zeyu Deng - wrote vector_from_matrix function(), helped debug and test code
Karthik Suryanarayana -

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.

Very nice!
Correct my comments and add test functions for remaining methods, e.g. test_set_ax.

geomstats/visualization/special_euclidean.py Outdated Show resolved Hide resolved
geomstats/visualization/special_euclidean.py Show resolved Hide resolved
geomstats/visualization/special_euclidean.py Outdated Show resolved Hide resolved
@monsij
Copy link
Collaborator

monsij commented Feb 14, 2023

Code linting fails due to special_orthogonal.py which was updated later in #1799 . I can open a separate PR to fix that 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants