sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.43k stars 479 forks source link

Lazy transition maps #33225

Open mjungmath opened 2 years ago

mjungmath commented 2 years ago

As discussed in #31249, a lazy implementation of charts or transition maps comes in handy in some situations as the number of transition maps explodes very easily.

In this ticket, we introduce a lazy implementation of transition maps that only initializes most attributes, especially the associated MultiCoordFunction and the inverse, if necessary.

CC: @tscrim @egourgoulhon @tobiasdiez @trevorkarn

Component: manifolds

Keywords: lazy, charts

Issue created by migration from https://trac.sagemath.org/ticket/33225

mjungmath commented 2 years ago
comment:1

We could try to speed up initialization by using slots. In my opinion, this is something we should use more frequently in manifolds anyway.

tscrim commented 2 years ago
comment:2

This seems like a good partial measure to do and is much smaller in scope than broad lazy objects. I also support the more targeted focus on the most computationally intensive part.

I would worry about doing __slots__ on a later ticket unless it is both easy and simple to do for this.

trevorkarn commented 2 years ago
comment:4

Are there any good examples of lazy objects in the existing codebase which we could use for inspiration?

mjungmath commented 2 years ago
comment:5

To name a few:

mjungmath commented 2 years ago
comment:6

Replying to @tscrim:

I would worry about doing __slots__ on a later ticket unless it is both easy and simple to do for this.

It shouldn't be very difficult. Actually it's quite straightforward. You only add a __slots__ attribute. Done. Keep in mind that the corresponding class has no __dict__ attribute then.

tscrim commented 2 years ago
comment:7

Replying to @mjungmath:

Replying to @tscrim:

I would worry about doing __slots__ on a later ticket unless it is both easy and simple to do for this.

It shouldn't be very difficult. Actually it's quite straightforward. You only add a __slots__ attribute. Done. Keep in mind that the corresponding class has no __dict__ attribute then.

That is not a trivial thing as it can have very far-reaching effects because you cannot free add attributes and it doesn't work well with @cached_method IIRC. It puts the class much more in the C++ realm (not necessarily a bad thing, but it can mean a lot of boilerplate code).

tscrim commented 2 years ago
comment:8

Replying to @trevorkarn:

Are there any good examples of lazy objects in the existing codebase which we could use for inspiration?

Two basic implementation ideas would be you either use an @lazy_attribute decorator for the relevant attribute(s) or you set it to None on __init__ and compute it when called for. (IMO, the first option is much better for Python both in terms of code maintenance, readability, and efficiency.) However, each individual implementation might require some variations.

trevorkarn commented 2 years ago
comment:9

Replying to @tscrim:

Replying to @trevorkarn:

Are there any good examples of lazy objects in the existing codebase which we could use for inspiration?

Two basic implementation ideas would be you either use an @lazy_attribute decorator for the relevant attribute(s) or you set it to None on __init__ and compute it when called for. (IMO, the first option is much better for Python both in terms of code maintenance, readability, and efficiency.) However, each individual implementation might require some variations.

Thanks so much!

Thinking about the original motivation for this, the Grassmannian, the problem we are looking to solve is the large overhead created by the creation of many charts. Looking at each individual chart and transition map it doesn't appear to me to have too much overhead. Are we looking to create something that is more of a "lazy atlas" or is a lazy transition map really what we want?

mjungmath commented 2 years ago
comment:10

Replying to @trevorkarn:

Thinking about the original motivation for this, the Grassmannian, the problem we are looking to solve is the large overhead created by the creation of many charts. Looking at each individual chart and transition map it doesn't appear to me to have too much overhead. Are we looking to create something that is more of a "lazy atlas" or is a lazy transition map really what we want?

I don't know for sure. My suggestion is to make a prun test on high-dimensional projective spaces and see what the culprits are.

Notice that one transition map is always accompanied by a MultiCoordFunction object, and another transition map, namely its inverse, again with a MultiCoordFunction object. However, there is no need to initialize the inverse on the fly when the formulas are known. My hope is that this leads to some speed-up.

Implementing __slots__ for charts might also help a little (speeds up initialization and access).

tscrim commented 2 years ago
comment:11

I would look at the sphere with perhaps stereographic and spherical coordinates. Really any manifold that has more complicated change of coordinates by default. (One way to get some speed might be to turn off checks; IDK if this is done already or not.) Please avoid doing slots on this ticket; that should be it's own ticket if we decide to do that.

egourgoulhon commented 2 years ago
comment:12

Replying to @mjungmath:

Replying to @trevorkarn:

Thinking about the original motivation for this, the Grassmannian, the problem we are looking to solve is the large overhead created by the creation of many charts. Looking at each individual chart and transition map it doesn't appear to me to have too much overhead. Are we looking to create something that is more of a "lazy atlas" or is a lazy transition map really what we want?

I don't know for sure. My suggestion is to make a prun test on high-dimensional projective spaces and see what the culprits are.

+1

egourgoulhon commented 2 years ago
comment:13

Replying to @tscrim:

I would look at the sphere with perhaps stereographic and spherical coordinates. Really any manifold that has more complicated change of coordinates by default. (One way to get some speed might be to turn off checks; IDK if this is done already or not.)

This is done already for spheres, cf. line 1029 of sage/manifolds/differentiable/examples/sphere.py:

spher_to_stereoN.set_inverse(*coordfunc, check=False)

Please avoid doing slots on this ticket; that should be it's own ticket if we decide to do that.

+1