search

sagemath / sage

Main repository of SageMath. Now open for Issues and Pull Requests.
https://www.sagemath.org
Other
1.21k stars 419 forks source link

MultiCoordFunction: Make it the element class of the free module over ChartFunctionRing #31982

Open mkoeppe opened 3 years ago

mkoeppe commented 3 years ago

Currently, MultiCoordFunction is not an element class.

sage: M = Manifold(2, 'M', structure='topological')
sage: X.<x,y> = M.chart()
sage: FR = X.function_ring()
sage: f = X.multifunction(x+y, sin(x*y), x^2 + 3*y); f
Coordinate functions (x + y, sin(x*y), x^2 + 3*y) on the Chart (M, (x, y))
sage: f.parent()
<class 'sage.manifolds.chart_func.MultiCoordFunction'>
sage: FR^3                                                 # with #31721
Ambient free module of rank 3 over Ring of chart functions on Chart (M, (x, y))
sage: f in FR^3
False

We change the function sage.modules.free_module.element_class so that it uses the element class Vector_symbolic_dense not only for free modules over the ring SR but also for free modules over any commutative ring whose base ring is SR.

In fact, we dispatch through a new method _free_module_element_class_dense. ChartFunction provides a specialized implementation, which makes sure that MultiCoordFunction (now a subclass of Vector_symbolic_dense) is used as the element class.

This simplifies the implementation of MultiCoordFunction and also makes the elementwise symbolic methods simplify etc. available.

Follow-up:

Depends on #31721 Depends on #31999

CC: @egourgoulhon @mjungmath @tscrim

Component: manifolds

Work Issues: rebase on #31999; fix pickling issue

Author: Matthias Koeppe

Branch/Commit: u/mkoeppe/multicoordfunction__make_it_the_element_class_of_the_free_module_over_chartfunctionring @ fb31802

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

mkoeppe commented 3 years ago

Branch: u/mkoeppe/multicoordfunction__make_it_the_element_class_of_the_free_module_over_chartfunctionring

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Commit: dbfe7ac

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Branch pushed to git repo; I updated commit sha1. New commits:

dbfe7acsrc/sage/modules/free_module.py: Update copyright years according to 'git blame -w --date=format:%Y src/sage/modules/free_module.py | sort -k2'
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Changed commit from dbfe7ac to cfd813b

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Branch pushed to git repo; I updated commit sha1. New commits:

cfd813bsage.modules.free_module.element_class: For rings with SR base ring, delegate to new method _free_module_element_class_dense
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Branch pushed to git repo; I updated commit sha1. New commits:

278aa2bCallableSymbolicExpressionRing_class: Fix doctest
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Changed commit from cfd813b to 278aa2b

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Branch pushed to git repo; I updated commit sha1. New commits:

7bbb5f8MultiCoordFunction: Make it an element class
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Changed commit from 278aa2b to 7bbb5f8

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Changed commit from 7bbb5f8 to 27e3889

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Branch pushed to git repo; I updated commit sha1. New commits:

fa5057cTrac 31721: `__pow__` method for parents
27422d331721: more comments
e8d3427clarify comment and doctest for multiple inheritance
b873bcbsome doc
27e3889Merge #31721
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Branch pushed to git repo; I updated commit sha1. New commits:

3c36bd2Eliminate use of MultiCoordFunction._functions
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Changed commit from 27e3889 to 3c36bd2

egourgoulhon commented 3 years ago
comment:8

Looks nice. However, the pickling of continuous maps fails with the new code for MultiCoordFunction. More precisely, the equality test of the pickling fails when attempting to compare the MultiCoordFunction's describing the continuous maps is a given pair of charts, with the error

File "/home/eric/sage/9.4.develop/local/lib/python3.8/site-packages/sage/manifolds/continuous_map.py", 
line 577, in __eq__
...
AttributeError: 'sage.modules.free_module_element.FreeModuleElement_generic_dense'
object  has no attribute 'expr'
mkoeppe commented 3 years ago
comment:9

Thanks for taking a look! Because of these errors I hadn't set the ticket to "needs review" yet. I'll revise it in the next few days, but help is welcome.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Branch pushed to git repo; I updated commit sha1. New commits:

49f63a4Vector_symbolic_dense: Fix pickling
dcdd412src/sage/manifolds/continuous_map.py: Update doctest output
fb31802src/sage/manifolds/chart.py: Update doctest output
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Changed commit from 3c36bd2 to fb31802

mkoeppe commented 3 years ago

Description changed:

--- 
+++ 
@@ -16,9 +16,11 @@

 We change the function `sage.modules.free_module.element_class` so that it uses the element class `Vector_symbolic_dense` not only for free modules over the ring `SR` but also for free modules over any commutative ring whose base ring is `SR`.

-This makes the elementwise symbolic methods `simplify` etc. available.
+In fact, we dispatch through a new method `_free_module_element_class_dense`. `ChartFunction` provides a specialized implementation, which makes sure that `MultiCoordFunction` (now a subclass of `Vector_symbolic_dense`) is used as the element class.

-We make `MultiCoordFunction` a subclass of `Vector_symbolic_dense`, moving the method `MultiCoordFunction.jacobian` to the superclass, making it more generally available.
+This simplifies the implementation of `MultiCoordFunction` and also  makes the elementwise symbolic methods `simplify` etc. available.

-Finally, we create a parent class `MultiCoordFunctionModule` (subclassing `FreeModule_ambient`) with element class `MultiCoordFunction`.

+Follow-up:
+- We could move the method `MultiCoordFunction.jacobian` to the superclass, making it more generally available.
+
mkoeppe commented 3 years ago

Author: Matthias Koeppe

mkoeppe commented 3 years ago
comment:12
sage -t --long --random-seed=0 src/sage/algebras/lie_algebras/quotient.py  # 1 doctest failed
sage -t --long --random-seed=0 src/sage/modules/free_module_element.pyx  # 1 doctest failed
mkoeppe commented 3 years ago
comment:13

I think I could use some help from a pickling expert here...

mjungmath commented 3 years ago
comment:14

I bet this is related to #31182.

mjungmath commented 3 years ago
comment:15

Okay, maybe not.

egourgoulhon commented 3 years ago
comment:16

Replying to @mkoeppe:

I think I could use some help from a pickling expert here...

Sorry I am definitely not such an expert and therefore cannot offer help here. Instead, let me ask about the motivation of this ticket. I understand that, from a mathematical point of view, MultiCoordFunction's belong to the free module FR^n (FR being the ring of chart functions associated to a given chart and n the manifold's dimension) and that implementing this simplifies (a little) the code by relying on some code already implemented for Vector_symbolic_dense. This, by itself, is a motivation. However, I would say this is not a strong one, because no use is made in the manifold code of the module structure of the set of MultiCoordFunction's over a given chart. Indeed, in the current implementation, the class MultiCoordFunction is used only to store

  1. the coordinate expression of a continous map in a given pair of charts;
  2. the transition map between two charts.

In both cases, there is no meaning in the basic module operations m1 + m2 and f*m1, where m1 and m2 are two MultiCoordFunction's and f is a chart function. Hence, the motivation to provide a parent with a module structure seems pretty weak, given the current use of the class MultiCoordFunction. So maybe it is not worth to pursue the effort and keep MultiCoordFunction as a simple technical storage class (by opposition to a "mathematical" class), not relying on sophisticated parts of Sage, with complicated pickling and possibly less efficient in terms of CPU time.

mkoeppe commented 3 years ago
comment:17

My main motivation for the present ticket is for defining (not necessarily smooth) vector-valued (and later matrix-valued #31988) functions on manifolds (for #31981). If another class is better suited for this, I'd be interested to know.

Some fixes here on the ticket are needed to make free module elements over the ring of coordinate functions work properly. Then I realized that MultiCoordFunction could just be the element class, giving some mild simplifications of the code. I agree that it is not essential to change MultiCoordFunction -- but whether I use that or a new class as the element class, I will need to fix the pickling of the elements.

mjungmath commented 3 years ago
comment:18

Regarding the pickling, can you spot after which commit the error occurred; more precisely, which lines caused it?

egourgoulhon commented 3 years ago
comment:19

Replying to @mkoeppe:

My main motivation for the present ticket is for defining (not necessarily smooth) vector-valued (and later matrix-valued #31988) functions on manifolds (for #31981).

Thanks for your quick reply. This sounds a strong motivation for a module structure!

If another class is better suited for this, I'd be interested to know.

Wouldn't a kind of MultiScalarField be better suited? In some sense, for vector-valued functions, this already exists through the class Components.

Some fixes here on the ticket are needed to make free module elements over the ring of coordinate functions work properly. Then I realized that MultiCoordFunction could just be the element class, giving some mild simplifications of the code. I agree that it is not essential to change MultiCoordFunction -- but whether I use that or a new class as the element class, I will need to fix the pickling of the elements.

Thanks for these explanations.

mkoeppe commented 3 years ago
comment:20

Replying to @egourgoulhon:

Replying to @mkoeppe:

My main motivation for the present ticket is for defining (not necessarily smooth) vector-valued (and later matrix-valued #31988) functions on manifolds (for #31981).

Thanks for your quick reply. This sounds a strong motivation for a module structure!

If another class is better suited for this, I'd be interested to know.

Wouldn't a kind of MultiScalarField be better suited? In some sense, for vector-valued functions, this already exists through the class Components.

You are probably right about this. I guess a tricky point is regarding the required smoothness of scalar fields. I like to think of the domain as a smooth manifold, but the functions that I want to define on them often are less smooth - typically only piecewise C1, with Lipschitz gradients. See for example https://www.cvxpy.org/api_reference/cvxpy.atoms.elementwise.html

mkoeppe commented 3 years ago

Changed dependencies from #31721 to #31721, #31999

mkoeppe commented 3 years ago

Work Issues: rebase on #31999; fix pickling issue

mkoeppe commented 2 years ago
comment:23

Setting a new milestone for this ticket based on a cursory review.