MultiCoordFunction: Make it the element class of the free module over ChartFunctionRing
mkoeppe opened this issue · 31 comments
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:
- We could move the method
MultiCoordFunction.jacobianto the superclass, making it more generally available.
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
Commit: dbfe7ac
Branch pushed to git repo; I updated commit sha1. New commits:
dbfe7ac | src/sage/modules/free_module.py: Update copyright years according to 'git blame -w --date=format:%Y src/sage/modules/free_module.py | sort -k2' |
Branch pushed to git repo; I updated commit sha1. New commits:
cfd813b | sage.modules.free_module.element_class: For rings with SR base ring, delegate to new method _free_module_element_class_dense |
Branch pushed to git repo; I updated commit sha1. New commits:
278aa2b | CallableSymbolicExpressionRing_class: Fix doctest |
Branch pushed to git repo; I updated commit sha1. New commits:
7bbb5f8 | MultiCoordFunction: Make it an element class |
Branch pushed to git repo; I updated commit sha1. New commits:
3c36bd2 | Eliminate use of MultiCoordFunction._functions |
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'
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.
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.
+Author: Matthias Koeppe
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
I think I could use some help from a pickling expert here...
Okay, maybe not.
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
- the coordinate expression of a continous map in a given pair of charts;
- 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.
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.
Regarding the pickling, can you spot after which commit the error occurred; more precisely, which lines caused it?
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
MultiCoordFunctioncould just be the element class, giving some mild simplifications of the code. I agree that it is not essential to changeMultiCoordFunction-- 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.
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
MultiScalarFieldbe better suited? In some sense, for vector-valued functions, this already exists through the classComponents.
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
Setting a new milestone for this ticket based on a cursory review.