Add visualization of Stiefel Manifold #1807

olokevin · 2023-01-29T23:25:58Z

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

Add visualization of Stiefel Manifold and corresponding unit test code

Issue

Additional context

review-notebook-app · 2023-01-29T23:26:02Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

codecov · 2023-01-29T23:31:53Z

Codecov Report

Merging #1807 (a0bbd4b) into master (1865fd0) will increase coverage by 6.82%.
The diff coverage is 87.98%.

❗ Current head a0bbd4b differs from pull request most recent head bd02e71. Consider uploading reports for the commit bd02e71 to get more accurate results

@@            Coverage Diff             @@
##           master    #1807      +/-   ##
==========================================
+ Coverage   80.29%   87.11%   +6.82%     
==========================================
  Files         121      125       +4     
  Lines       12830    12809      -21     
==========================================
+ Hits        10301    11157     +856     
+ Misses       2529     1652     -877

Flag	Coverage Δ
numpy	`87.11% <87.98%> (?)`
pytorch	`?`

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

Impacted Files	Coverage Δ
geomstats/visualization/stiefel_manifold.py	`87.98% <87.98%> (ø)`
geomstats/geometry/sub_riemannian_metric.py	`30.40% <0.00%> (-62.74%)`	⬇️
geomstats/geometry/pullback_metric.py	`32.05% <0.00%> (-58.56%)`	⬇️
geomstats/learning/geodesic_regression.py	`31.17% <0.00%> (-48.70%)`	⬇️
...eomstats/information_geometry/fisher_rao_metric.py	`21.88% <0.00%> (-46.87%)`	⬇️
geomstats/geometry/fiber_bundle.py	`66.67% <0.00%> (-28.20%)`	⬇️
geomstats/geometry/connection.py	`76.98% <0.00%> (-17.76%)`	⬇️
geomstats/geometry/riemannian_metric.py	`78.63% <0.00%> (-10.34%)`	⬇️
geomstats/learning/geometric_median.py	`92.31% <0.00%> (-2.56%)`	⬇️
geomstats/_backend/pytorch/_common.py
... and 56 more

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

ninamiolane

Rename the ipynb to stiefel.ipynb

ninamiolane · 2023-02-13T16:52:43Z

geomstats/visualization/stiefel_manifold.py

@@ -0,0 +1,450 @@
+"""Stiefel manifold module."""


Rename file to stiefel.py

ninamiolane · 2023-02-13T16:52:58Z

geomstats/visualization/stiefel_manifold.py

+from geomstats.visualization import Sphere
+
+# %%
+"""


Put this in the docstring of the class StiefelSphere

ninamiolane · 2023-02-13T16:53:07Z

geomstats/visualization/stiefel_manifold.py

+from geomstats.geometry.hypersphere import Hypersphere
+from geomstats.visualization import Sphere
+
+# %%


Remove this unnecessary comment

ninamiolane · 2023-02-13T16:53:36Z

tests/tests_geomstats/test_visualization_stiefel_manifold.py

@@ -0,0 +1,149 @@
+"""Stiefel manifold visualization test module."""


rename this file: test_visualization_stiefel.py

ninamiolane

Thanks, looking great!

Please submit a review of this PR with the following changes:

Instead of copy-pasting code from the visualization of Kendall shape spaces, you should import that code and have your methods call their methods. You will be, in effect, wrapping that code into a new object, which is cleaner.
Address DeepSource issues.
See my comment regarding the line that you can add to avoid getting the errors in pytorch.
Try adding a section "Setup" to your notebook, as in this example, so that the import error goes away.

ninamiolane · 2023-02-13T17:01:42Z

geomstats/visualization/stiefel_manifold.py

+
+# %%
+"""
+    Consider <n=3, p=1> Stiefel Manifold.


Should this be n=3, p=2? A pair of two orthonormal vectors in 3D? Double check

ninamiolane · 2023-02-13T17:02:07Z

geomstats/visualization/stiefel_manifold.py

+
+class StiefelSphere:
+    """
+    Class used to plot something in Stiefel shape space of 2D triangles.


Add reference showing that the stiefel space St(2, 3) is indeed the shape space of triangles.
Look at other code in geomstats repo to see how to properly add a reference in a docstring

ninamiolane · 2023-02-13T17:02:43Z

geomstats/visualization/stiefel_manifold.py

+
+    References
+    ----------
+    Absil, Pierre-Antoine. "Stiefel Manifolds and their Applications." (2009)


Format this reference, using examples from other docstring in geomstats' code.
Otherwise, this will not render properly on the documentation website of geomstats, geomstats.ai

ninamiolane · 2023-02-13T17:02:57Z

geomstats/visualization/stiefel_manifold.py

+
+class StiefelSphere:
+    """
+    Class used to plot something in Stiefel shape space of 2D triangles.


The one liner of the docstring should come direclty after """

ninamiolane · 2023-02-13T17:03:09Z

geomstats/visualization/stiefel_manifold.py

+
+    Attributes
+    ----------
+      ax : NoneType


No indent before ax

ninamiolane · 2023-02-13T17:07:50Z

tests/tests_geomstats/test_visualization_stiefel_manifold.py

+matplotlib.use("Agg")  # NOQA
+
+
+class TestStiefelManifold(tests.conftest.TestCase):


This should be TestStiefelSphere or TestStiefelCircle
--> one test class per class being tested, and the classs names should match

Then, the names of the test methods do not need to be prefixed by "test_stiefel_circle" or "test_stiefel_sphere" because they are already in a class named TestStiefelCircle or TestStiefelSphere

ninamiolane · 2023-02-13T17:08:14Z

tests/tests_geomstats/test_visualization_stiefel_manifold.py

+        self.v21 = Stiefel(2, 1)
+        self.v22 = Stiefel(2, 2)
+        self.v31 = Stiefel(3, 1)
+        self.St_sph = StiefelSphere()


no uppercase letters in python, unless the variable is a Class, which it is not here (it is an object)

ninamiolane · 2023-02-13T17:08:46Z

tests/tests_geomstats/test_visualization_stiefel_manifold.py

+        self.St_cir.add_points(group1_points_to_circle[:, :, 0])
+        self.St_cir.draw_curve(color="b", lw=3, label="geodesic line")
+
+    def test_stifel_circle_draw_tangent_space(self):


NIT: stiefel not stifel

ninamiolane · 2023-02-13T17:11:05Z

tests/tests_geomstats/test_visualization_stiefel_manifold.py

+        self.St_sph.clear_points()
+        self.St_sph.draw_points(self.sph_ax)
+
+    def test_stiefel_sphere_draw_mesh(self):


Put this line above the test functions that fail in pytorch:
@geomstats.tests.np_only

after importing geomstats.tests at the top of the file.

The github actions errors will go away

ninamiolane · 2023-02-13T17:11:21Z

tests/tests_geomstats/test_visualization_stiefel_manifold.py

+    def test_stiefel_circle_plot_rendering(self):
+        """Test drawing the manifold with regularly sampled data."""
+        self.St_cir.plot_rendering(self.v21, 100)
+        # self.St_cir.plot_rendering(self.v22, 100)


Rm leftover comment

review-notebook-app · 2023-02-13T17:17:55Z

View / edit / reply to this conversation on ReviewNB

ninamiolane commented on 2023-02-13T17:17:55Z
----------------------------------------------------------------

Rename this notebook to stiefel.py (no _)

review-notebook-app · 2023-02-13T17:17:56Z

View / edit / reply to this conversation on ReviewNB

ninamiolane commented on 2023-02-13T17:17:56Z
----------------------------------------------------------------

Change "In this presentation" to "Here,"

Change "This rest of the presentation is" to "This notebook is"

review-notebook-app · 2023-02-13T17:17:57Z

View / edit / reply to this conversation on ReviewNB

ninamiolane commented on 2023-02-13T17:17:57Z
----------------------------------------------------------------

Line #2.    # begin

Remove the comments here (they are unnecessary)

review-notebook-app · 2023-02-13T17:17:58Z

View / edit / reply to this conversation on ReviewNB

ninamiolane commented on 2023-02-13T17:17:58Z
----------------------------------------------------------------

Line #1.    ###########################################################################

Remove unnecessary comments in the code, and put a markdown block with clean text instead.

review-notebook-app · 2023-02-13T17:17:59Z

View / edit / reply to this conversation on ReviewNB

ninamiolane commented on 2023-02-13T17:17:59Z
----------------------------------------------------------------

Ditto: remove comments and put a well-written text in a markdown cell instead

review-notebook-app · 2023-02-13T17:18:00Z

View / edit / reply to this conversation on ReviewNB

ninamiolane commented on 2023-02-13T17:18:00Z
----------------------------------------------------------------

Ditto re comments and text

review-notebook-app · 2023-02-13T17:18:01Z

View / edit / reply to this conversation on ReviewNB

ninamiolane commented on 2023-02-13T17:18:00Z
----------------------------------------------------------------

NIT: implemented

olokevin added 3 commits January 28, 2023 23:08

add visualization of stiefel_manifole

a7d857c

no problems

75bc490

fixed problem

119c654

fixed flake8 error

29978e4

olokevin added 5 commits January 29, 2023 16:45

fixed test plot rendering problem

1b789d2

fix flake8 prob

b5a56c6

fix indent error

0118bb7

reformatted

6607aea

black ipynb

cd2fde9

olokevin force-pushed the yq_zhao branch from c89e0d7 to cd2fde9 Compare January 30, 2023 09:43

olokevin added 2 commits January 30, 2023 12:11

Formatted

bf41b80

need to isort then black

bd02e71

olokevin force-pushed the yq_zhao branch from a0bbd4b to bd02e71 Compare January 31, 2023 00:47

olokevin changed the title ~~Yq zhao~~ Add visualization of Stiefel Manifold Jan 31, 2023

ninamiolane added the ece594n label Feb 1, 2023

ninamiolane reviewed Feb 13, 2023

View reviewed changes

ninamiolane requested changes Feb 13, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add visualization of Stiefel Manifold #1807

Add visualization of Stiefel Manifold #1807

olokevin commented Jan 29, 2023 •

edited

review-notebook-app bot commented Jan 29, 2023

codecov bot commented Jan 29, 2023 •

edited

ninamiolane left a comment

ninamiolane Feb 13, 2023

ninamiolane Feb 13, 2023 •

edited

ninamiolane Feb 13, 2023

ninamiolane Feb 13, 2023

ninamiolane left a comment •

edited

ninamiolane Feb 13, 2023

ninamiolane Feb 13, 2023

ninamiolane Feb 13, 2023

ninamiolane Feb 13, 2023

ninamiolane Feb 13, 2023

ninamiolane Feb 13, 2023

ninamiolane Feb 13, 2023

ninamiolane Feb 13, 2023

ninamiolane Feb 13, 2023

ninamiolane Feb 13, 2023

ninamiolane Feb 13, 2023

review-notebook-app bot commented Feb 13, 2023 •

edited

review-notebook-app bot commented Feb 13, 2023 •

edited

review-notebook-app bot commented Feb 13, 2023 •

edited

review-notebook-app bot commented Feb 13, 2023 •

edited

review-notebook-app bot commented Feb 13, 2023 •

edited

review-notebook-app bot commented Feb 13, 2023 •

edited

review-notebook-app bot commented Feb 13, 2023 •

edited

		@@ -0,0 +1,149 @@
		"""Stiefel manifold visualization test module."""

		matplotlib.use("Agg") # NOQA


		class TestStiefelManifold(tests.conftest.TestCase):

Add visualization of Stiefel Manifold #1807

Are you sure you want to change the base?

Add visualization of Stiefel Manifold #1807

Conversation

olokevin commented Jan 29, 2023 • edited

Checklist

Description

Issue

Additional context

review-notebook-app bot commented Jan 29, 2023

codecov bot commented Jan 29, 2023 • edited

Codecov Report

ninamiolane left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ninamiolane Feb 13, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ninamiolane left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

review-notebook-app bot commented Feb 13, 2023 • edited

review-notebook-app bot commented Feb 13, 2023 • edited

review-notebook-app bot commented Feb 13, 2023 • edited

review-notebook-app bot commented Feb 13, 2023 • edited

review-notebook-app bot commented Feb 13, 2023 • edited

review-notebook-app bot commented Feb 13, 2023 • edited

review-notebook-app bot commented Feb 13, 2023 • edited

olokevin commented Jan 29, 2023 •

edited

codecov bot commented Jan 29, 2023 •

edited

ninamiolane Feb 13, 2023 •

edited

ninamiolane left a comment •

edited

review-notebook-app bot commented Feb 13, 2023 •

edited

review-notebook-app bot commented Feb 13, 2023 •

edited

review-notebook-app bot commented Feb 13, 2023 •

edited

review-notebook-app bot commented Feb 13, 2023 •

edited

review-notebook-app bot commented Feb 13, 2023 •

edited

review-notebook-app bot commented Feb 13, 2023 •

edited

review-notebook-app bot commented Feb 13, 2023 •

edited