Reserve scale parameter to ScalarProductMetric

[johnharveymath]

Closes #1757 and closes #1758 by removing the scale parameter from various manifolds.

These are the Hyperbolic, HPDMatrices and PoincarePolydisk manifolds, along with all their related classes.

@YannCabanes The tests are failing on PoincarePolydisk and I don't understand the objects well enough to remedy the issue. Maybe you could have a look?

[luisfpereira]

Thanks @johnharveymath! Please take a look to my comments (most of them are little details).

I'll try to understand the tests that are failing. @YannCabanes, your feedback and help would be of great value here, as most of scale related things were developed by you.

[johnharveymath]

Seems like for most classes it was the distance that was scaled by self.scale, except for PoincareHalfSpace where it was the inner product that got scaled. So I think it's great that we're taking this out as there was clearly some inconsistency going on.

[codecov]

Codecov Report

Merging #1761 (93c4727) into master (3543efa) will increase coverage by 9.70%.
The diff coverage is 79.77%.

@@            Coverage Diff             @@
##           master    #1761      +/-   ##
==========================================
+ Coverage   80.90%   90.60%   +9.70%     
==========================================
  Files         120      126       +6     
  Lines       12856    13199     +343     
==========================================
+ Hits        10400    11957    +1557     
+ Misses       2456     1242    -1214     

Flag	Coverage Δ
numpy	87.60% <79.77%> (?)
pytorch	80.77% <79.77%> (-0.12%)	⬇️

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

Impacted Files	Coverage Δ
geomstats/geometry/hyperbolic.py	75.00% <ø> (ø)
geomstats/geometry/riemannian_metric.py	84.83% <0.00%> (+7.59%)	⬆️
geomstats/geometry/poincare_ball.py	92.03% <62.50%> (-1.35%)	⬇️
geomstats/geometry/_hyperbolic.py	96.94% <66.67%> (-0.28%)	⬇️
geomstats/geometry/siegel.py	98.11% <71.43%> (-0.94%)	⬇️
geomstats/geometry/complex_poincare_disk.py	96.50% <75.00%> (-3.50%)	⬇️
geomstats/geometry/poincare_half_space.py	93.34% <75.00%> (-4.28%)	⬇️
geomstats/geometry/positive_reals.py	95.66% <75.00%> (-2.12%)	⬇️
geomstats/geometry/hpd_matrices.py	95.50% <80.00%> (+8.67%)	⬆️
geomstats/geometry/hyperboloid.py	83.63% <90.00%> (-0.86%)	⬇️
... and 50 more

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

[YannCabanes]

Hello @johnharveymath and @LPereira95,
With my notations, the scale parameter means multiplying the distance by the scale parameter, which means multiplying:

• dist --> (multiply by) scale

• norm --> scale

• metric_matrix --> scale ** 2

• inner_product --> scale ** 2

• squared_norm --> scale ** 2

• squared_dist --> scale ** 2

However, I see that in the current master branch, the file product_riemannian_metric.py is using a different convention for metric_matrix and inner_product as they are only multiplied by scale and not scale ** 2.

I am currently looking at the how the scale influences the curvature, sectional_curvature, riemann_tensor...

[luisfpereira]

Thanks for the feedback @YannCabanes.

I think the implementation of ScalarProductMatrix is consistent. Which probably means we just need to remove **.5 as you say.

Nevertheless, are you sure about the correctness of TestPoincarePolydiskMetric.test_product? I can't understand why the distances should be the same for these cases. Could you please explain me?

[YannCabanes]

Hello @johnharveymath and @LPereira95,
I just did a complete review of the influence of the scale parameter.
The scale parameter (related to the metric) has no influence on any function defined in the file connection.py (christoffels, exp, log, riemann_tensor, curvature, ricci_tensor, geodesic...).
The scale parameter influences the methods defined in the file riemannian_metric.py.
Using the convention in which the distance is multiplied by the scale parameter and the metric_matrix by scale ** 2, I now list the influence of the scale parameter on each method of the class RiemannianMetric:

• metric_matrix --> * scale ** 2
• cometric_matrix --> / scale ** 2
• inner_product_derivative_matrix --> * scale ** 2
• christoffels --> no influence
• inner_product --> * scale ** 2
• inner_coproduct --> / scale ** 2
• hamiltonian --> / scale ** 2
• squared_norm --> * scale ** 2
• norm --> * scale
• normalize --> / scale
• random_unit_tangent_vec --> / scale
• squared_dist --> * scale ** 2
• dist --> * scale
• dist_broadcast --> * scale
• dist_pairwise --> * scale
• diameter --> * scale
• closest_neighbor_index --> no influence
• normal_basis --> / scale
• covariant_riemann_tensor --> * scale ** 2
• sectional_curvature --> / scale ** 2
• scalar_curvature --> / scale ** 2

[YannCabanes]

Hello @LPereira95,
About the TestPoincarePolydiskMetric.test_product_distance, you are right, distance_two_disks is different from distance_single_disk. This is a change introduced in the current PR.
Normally, when point_a and point_b are duplicated in each disks, we should have:

• distance_single_disk = distance HyperboloidMetric(2)
• distance_two_disks = 3 ** 0.5 * distance_single_disk
• For n disks, we should have: distance_n_disks = ((n * (n+1) / 2) ** 0.5 * distance_single_disk

[johnharveymath]

Hello @LPereira95, About the TestPoincarePolydiskMetric.test_product_distance, you are right, distance_two_disks is different from distance_single_disk. This is a change introduced in the current PR. Normally, when point_a and point_b are duplicated in each disks, we should have:

* `distance_single_disk` = distance HyperboloidMetric(2)

* `distance_two_disks = 3 ** 0.5 * distance_single_disk`

* For n disks, we should have: `distance_n_disks = ((n * (n+1) / 2) ** 0.5 * distance_single_disk`

Thanks for the explanation - this was caused by my ignorance of the geometry. I though the 3 had something to do with the scale parameter. This test passes now.

[johnharveymath]

I just did a complete review of the influence of the scale parameter.

I'm glad you did, @YannCabanes! Your review reveals that some parameters were being divided by scale ** 2 instead of gs.sqrt(scale). I have opened PR #1763 to correct these.

As I mentioned earlier, it's definitely best to just have one implementation that is in ScalarProductMetric. This is because we did actually have some inconsistencies about how scale was treated in different classes. @alebrigant and I chose this particular implementation because we are expressing it as multiplying the metric (ie, the matrix, or the (0, 2)-tensor if you prefer) by a scalar. Your implementation is far more natural (this is how we think about scaling a space) but I think that, given that we can now write h = 2 * g, it is better to go with the more mathematically formalistic approach.

[johnharveymath]

OK - I think we're ready to go.

In summary, this PR:

• removes the scale parameter from _hyperbolic and its children, Hyperboloid, PoincareBall, PoincareHalfSpace as well as from ComplexPoincareDisk, HPDMatrices, PositiveReals and Siegel.
• adds an informative error message for users who attempt to initialize these manifolds or their metrics while using a scale parameter
• makes ScalarProductMetric a child of RiemannianMetric so that it is possible to write manifold.metric = 2.0 * manifold.metric

[luisfpereira]

Great work @johnharveymath and @YannCabanes! I'm merging now.

@johnharveymath, just two last comments for future reference:

1. I think we should remove the error messages for scale soon. I thought about removing it before merging, but I see we should not do it as our inits accept kwargs (which mean code with scale will not fail, but will provide unexpected results). Very soon I want to remove **kwargs from all the inits, so we can remove this kind of errors (python error messages will be good enough then).

2. I think ScalarProductMetric shouldn't inherit from RiemannianMetric. The reason is that by doing that we lose control over the methods we are providing: e.g. in init we do not wrap private methods of underlying_metric, but we inherit them from RiemannianMetric, without providing proper scaling (e.g. imagine in the future we implement _inner_product for some reason, it will become available for ScalarProductMetric). What do you think? We can simply implement the multiplication in ScalarProductMetric also.

[johnharveymath]

That's makes sense to me. The only issue is that you cannot actually assign the `ScalarProductMetric` object to be an attribute of a `Manifold`, ie, you cannot do `manifold.metric = 2.0 * manifold.metric`. I have changed `manifold.py` in one additional commit to permit this. I know there has been discussion about making `manifold.metric` a read-only object in future, but it seems to me that this might be one time you want to be able edit it.

…

On Thu, Dec 15, 2022 at 1:53 PM Luís F. Pereira ***@***.***> wrote: Great work @johnharveymath <https://github.com/johnharveymath> and @YannCabanes <https://github.com/YannCabanes>! I'm merging now. @johnharveymath <https://github.com/johnharveymath>, just two last comments for future reference: 1. I think we should remove the error messages for scale soon. I thought about removing it before merging, but I see we should not do it as our inits accept kwargs (which mean code with scale will not fail, but will provide unexpected results). Very soon I want to remove **kwargs from all the inits, so we can remove this kind of errors (python error messages will be good enough then). 2. I think ScalarProductMetric shouldn't inherit from RiemannianMetric. The reason is that by doing that we lose control over the methods we are providing: e.g. in *init* we do not wrap private methods of underlying_metric, but we inherit them from RiemannianMetric, without providing proper scaling (e.g. imagine in the future we implement _inner_product for some reason, it will become available for ScalarProductMetric). What do you think? We can simply implement the multiplication in ScalarProductMetric also. — Reply to this email directly, view it on GitHub <#1761 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AOQUDAOU57G4CD4TI3NIJHDWNMIGNANCNFSM6AAAAAAS5MJOGI> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[luisfpereira]

Thanks @johnharveymath!

I added multiplication to ScalarProductMetric (because it is not being inherited). It is code duplication, but I think for a good reason here.

Merging now...

[johnharveymath]

Well spotted. Thanks for all your advice and help on this one.

…

On Thu, Dec 15, 2022 at 2:23 PM Luís F. Pereira ***@***.***> wrote: Thanks @johnharveymath <https://github.com/johnharveymath>! I added multiplication to ScalarProductMetric (because it is not being inherited). It is code duplication, but I think for a good reason here. Merging now... — Reply to this email directly, view it on GitHub <#1761 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AOQUDAPPJZHAA64ID4LZHNDWNMLU5ANCNFSM6AAAAAAS5MJOGI> . You are receiving this because you were mentioned.Message ID: ***@***.***>