8000 Add method to change the lists in `scalar_product_metric.py` by johnharveymath · Pull Request #1964 · geomstats/geomstats · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Add method to change the lists in scalar_product_metric.py #1964

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
Mar 1, 2024
Merged

Add method to change the lists in scalar_product_metric.py #1964

merged 6 commits into from
Mar 1, 2024

Conversation

johnharveymath
Copy link
Collaborator
@johnharveymath johnharveymath commented Feb 23, 2024 •
edited
Loading

A new class method is added to ScalarProductMetric called add_scaled_method which allows the user to configure how a method from RiemannianMetric should be scaled. The lists become class variables to allow them to be edited.

This fixes #1760.

###Tests

The added feature does not seem amenable to unit testing, but the following code does demonstrate that it works:

from geomstats import geomstats
from geomstats.geometry.scalar_product_metric import ScalarProductMetric
from geomstats.geometry.hypersphere import Hypersphere

sphere = Hypersphere(2)
point = sphere.random_point(1)
print(sphere.metric.injectivity_radius(point))

new_metric = 2.0 * sphere.metric
print(new_metric.injectivity_radius(point))

ScalarProductMetric.add_scaled_method('injectivity_radius', 'linear')
new_metric = 2.0 * sphere.metric
print(new_metric.injectivity_radius(point))

This has output

3.141592653589793
3.141592653589793
6.283185307179586

as expected.

Questions

  1. Should we add a functionality to remove items from the lists or to restore the 'factory default'?
  2. Are we happy with allowing methods which are not explicitly handled to just silently go through untransformed? Maybe we should at least log that it's happening?

@codecov Codecov
Copy link
codecov bot commented Feb 23, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 65.00000% with 14 lines in your changes are missing coverage. Please review.

Project coverage is 91.23%. Comparing base (0b37cae) to head (4a967d6).
Report is 19 commits behind head on main.

Files Patch % Lines
geomstats/geometry/scalar_product_metric.py 65.00% 14 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1964      +/-   ##
==========================================
- Coverage   91.63%   91.23%   -0.39%     
==========================================
  Files         151      147       -4     
  Lines       13865    13669     -196     
==========================================
- Hits        12704    12470     -234     
- Misses       1161     1199      +38
Flag Coverage Δ
autograd ?
numpy 89.22% <65.00%> (-0.14%) ⬇️
pytorch 87.22% <65.00%> (+1.45%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@luisfpereira luisfpereira self-requested a review February 23, 2024 16:59
luisfpereira
luisfpereira requested changes Feb 28, 2024
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.

Thanks @johnharveymath!

The idea is very good. I'm just wondering if we can keep the previous function-based approach and add a method called e.g. register_scaled_method that does exactly what add_scaled_method is doing, but in a more transparent way (by being outside of the class). Is there any technical reason for such a solution not work?

@johnharveymath
Copy link
Collaborator Author
johnharveymath commented Feb 29, 2024 •
edited
Loading

Are you thinking something that would be used like this

from geomstats.geometry import register_scaled_method

register_scaled_method("injectivity_radius", "linear")

and behind the scenes it is editing ScalarProductMetric.LINEAR_LIST and so on? I don't see why not - the simplest approach would simply be that this method actually just wraps ScalarProductMetric.add_scaled_metric.

Another approach would be to have some sort of singleton class ScaledMethodsRegistry that holds all the lists. Then register_scaled_method will change the attributes of that class. I understand that it is more or less the same thing to just put all the lists back at module level where they were to being with. However, I've read that if there are multiple threads the different threads can import the module separately, so that one thread would not recognise that another thread had changed the lists. Is this a concern for us?

Whichever way we do this, a key advantage is that we could put a call to register_scaled_method inside the __init__ for a lot of our existing RiemannianMetric child classes to handle any new methods, such as Hypersphere.metric.injectivity_radius.

@luisfpereira
Copy link
Collaborator

Exactly that @johnharveymath, very nice!

I like your singleton class idea very much (even though threading is not necessarily something we care about). The other advantage of this strategy I can see is that everything becomes encapsulated 8000 in an object, so we know clearly where to look for. I sugggest to make this class private (_ScaledMethodsRegistry).

@johnharveymath
Copy link
Collaborator Author
johnharveymath commented Mar 1, 2024 •
edited
Loading

Here we go. It seemed to make sense to put _get_scaling_factor into the new class as well. Conceptually, this class now holds all information to do with which methods can be handled and how to handle them. I put _RESERVED_NAMES in here as well.

The result is that ScalarProductMetric now goes to _Scaled_Methods_Registry every time it wants to know anything about a method, either accessing one of its private methods or directly accessing its private variable _RESERVED_NAMES. I'm not 100% of whether that's a nice structure.

Then, as for where the function register_scaled_method should go, I realised that we usually keep everything a little deep in the namespace and you have to go to the correct module to pull it out. Doing from geomstats.geometry import register_scaled_method would be a bit out of the ordinary. So it is going to be from geomstats.geometry.scalar_product_metric import register_scaled_method. I think that's fine, as it's not going to see a lot of use. I think the most use it will get will be internal. Once we're happy with this I can add it to a few classes where I see the need, either in this PR or in a new one.

luisfpereira
luisfpereira approved these changes Mar 1, 2024
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.

I think the design is very clean @johnharveymath. From my side we can merge as it it. (I suggest to register methods in another PR.)

@luisfpereira luisfpereira merged commit 83b16de into geomstats:main Mar 1, 2024
@johnharveymath johnharveymath deleted the change_scaling_lists branch March 1, 2024 16:42
@luisfpereira luisfpereira mentioned this pull request Apr 18, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@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.

Add method to change the lists in scalar_product_metric.py
2 participants
@johnharveymath @luisfpereira
0