Skip to content
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.

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

VectorFieldModule, TensorFieldModule, DiffFormModule: Add methods tensor_product, tensor_power, update category of TensorFieldModule #34589

Closed
mkoeppe opened this issue Sep 26, 2022 · 58 comments

Comments

@mkoeppe
Copy link
Contributor

mkoeppe commented Sep 26, 2022

... as previously done for FiniteRankFreeModule etc.

We introduce abstract base classes ReflexiveModule_* to unify the implementations of VectorFieldModule... and FiniteRankFreeModule/VectorFieldFreeModule...

Follow-up: #34621 Method dual_pairing for modules in sage.tensor

CC: @egourgoulhon @tscrim @mjungmath

Component: manifolds

Author: Matthias Koeppe

Branch/Commit: 74601e6

Reviewer: Eric Gourgoulhon

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

@mkoeppe mkoeppe added this to the sage-9.8 milestone Sep 26, 2022
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 26, 2022

Dependencies: #34448

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 26, 2022

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 26, 2022

Commit: 31b17b5

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 26, 2022

comment:3

Probably best to create a class VectorFieldModule_abstract


Last 10 new commits:

620df16src/sage/tensor/modules/tensor_free_submodule.py: Remove tensor_factors, add doctest for construction
4c9be6bsrc/sage/manifolds/differentiable/diff_form_module.py: doc: add that the case p=1 of M parallelizable is implemented via VectorFieldDualFreeModule
06710c1src/sage/manifolds/differentiable/diff_form_module.py, vectorfield_module.py: Update copyright using 'git blame -w --date=format:%Y FILE.py | sort -k2', add to AUTHORS
bb614c1Merge #34501
4a4a29dMerge tag '9.7' into t/34448/put_tensor_modules_of_finiterankfreemodule_in_modules___tensorproducts__
1f4fb90Merge tag '9.8.beta0' into t/34448/put_tensor_modules_of_finiterankfreemodule_in_modules___tensorproducts__
7c04959src/sage/manifolds/differentiable/diff_form_module.py: Fix typo in docstring
fc31cbfMerge #34501
f0def92src/sage/tensor/modules/tensor_free_module.py: Remove redundant method
31b17b5WIP

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 27, 2022

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

e10f9cfsrc/sage/manifolds/differentiable/vectorfield_module.py: Revert change to VectorFieldModule.tensor (not needed yet)
ff00c11VectorFieldModule: Add tensor_product, tensor_power
8d753dcsrc/sage/manifolds/differentiable/vectorfield_module.py (VectorFieldModule_abstract): New

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 27, 2022

Changed commit from 31b17b5 to 8d753dc

@mkoeppe mkoeppe changed the title VectorFieldModule, TensorFieldModule, DiffFormModule: Add methods tensor_product, tensor_power VectorFieldModule, TensorFieldModule, DiffFormModule: Add methods tensor_product, tensor_power, add TensorFieldModule.tensor_factors Sep 27, 2022
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 27, 2022

Changed commit from 8d753dc to 6bada9f

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 27, 2022

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

2830ebcTensorFreeModule.tensor_factors: Go through methods tensor_type, base_module
6bada9fTensorFieldModule.tensor_factors: New

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 28, 2022

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

347ffc0TensorFieldModule: Put it in the category Modules.TensorProducts

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 28, 2022

Changed commit from 6bada9f to 347ffc0

@mkoeppe mkoeppe changed the title VectorFieldModule, TensorFieldModule, DiffFormModule: Add methods tensor_product, tensor_power, add TensorFieldModule.tensor_factors VectorFieldModule, TensorFieldModule, DiffFormModule: Add methods tensor_product, tensor_power, update category of TensorFieldModule Sep 28, 2022
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 28, 2022

Changed commit from 347ffc0 to 7194893

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 28, 2022

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

7194893src/sage/manifolds/differentiable/tensorfield_module.py: Update copyright according to git blame -w --date=format:%Y FILE | sort -k2

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 28, 2022

Author: Matthias Koeppe

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 29, 2022

comment:11

This new base class is of course not very pretty

+class VectorFieldModule_abstract(UniqueRepresentation, Parent):
+    r"""
+    Abstract base class for modules of vector fields.
+    """
+
+    tensor_power = FiniteRankFreeModule_abstract.tensor_power
+
+    tensor_product = FiniteRankFreeModule_abstract.tensor_product
+
+    tensor = FiniteRankFreeModule_abstract.tensor

It could be better to have a common base class for VectorFieldModule_abstract and FiniteRankFreeModule_abstract.

Perhaps that could be called ModuleWithDual_abstract? (see also #13372)

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 29, 2022

comment:12

Algebra question - do we know / do we assume that a VectorFieldModule is reflexive?

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 29, 2022

comment:13

(in this case, we could call it ReflexiveModule_abstract - for such modules the pair tensor_type makes sense.)

@egourgoulhon
Copy link
Member

comment:14

Replying to Matthias Köppe:

Algebra question - do we know / do we assume that a VectorFieldModule is reflexive?

I would say that the vector field modules are indeed reflexive for the cases we can treat with Sage, namely those for which the manifold M can be split into a finite number of parallelizable manifolds N_i, i.e. there is a finite open cover of M, (N_i)_{1\leq i \leq p} (p finite), such that for each i, N_i is parallelizable. Let then consider an element f of X(M)**, where X(M) is the Coo(M)-module of vector fields on M. For a in X(M)*, f(a) can be reduced to f_i(a_i) on each N_i. Since N_i is parallelizable, X(N_i) is a free module of finite rank and thus reflexive. There exists then a unique v_i in X(N_i) such that f_i(a_i) = a_i(v_i). Then the vector field v in X(M) defined as being equal to v_i on each N_i obeys f(a) = a(v), which shows that the canonical map X(M) --> X(M)** is bijective. This is a first quick thought and this should be double checked...

@egourgoulhon
Copy link
Member

comment:15

Replying to Matthias Köppe:

(in this case, we could call it ReflexiveModule_abstract - for such modules the pair tensor_type makes sense.)

Indeed, if we confirm that the vector field modules are reflexive.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 29, 2022

comment:16

Replying to Eric Gourgoulhon:

I would say that the vector field modules are indeed reflexive for the cases we can treat with Sage, namely those for which the manifold M can be split into a finite number of parallelizable manifolds N_i, i.e. there is a finite open cover of M, (N_i)_{1\leq i \leq p} (p finite), such that for each i, N_i is parallelizable.

Is this splitting into parallelizable manifolds explicitly done somewhere in the code?

I guess a subtlety is that in sage.manifolds a manifold M is thought to be immutable but new submanifolds on it can be declared imperatively. At any point, of course only finitely many submanifolds have been declared in M as a Python object, but it's not clear that M as a mathematical object is really assumed to be covered by only a finite number of parallelizable submanifolds...

@egourgoulhon
Copy link
Member

comment:17

Replying to Matthias Köppe:

Replying to Eric Gourgoulhon:

I would say that the vector field modules are indeed reflexive for the cases we can treat with Sage, namely those for which the manifold M can be split into a finite number of parallelizable manifolds N_i, i.e. there is a finite open cover of M, (N_i)_{1\leq i \leq p} (p finite), such that for each i, N_i is parallelizable.

Is this splitting into parallelizable manifolds explicitly done somewhere in the code?

Not explicitly, but DifferentiableManifold's have the attribute _parrelizable_parts; for instance:

sage: M = manifolds.Sphere(2, coordinates='stereographic')
sage: M._parallelizable_parts
{Open subset S^2-{NP,SP} of the 2-sphere S^2 of radius 1 smoothly embedded in the Euclidean space E^3,
 Open subset S^2-{NP} of the 2-sphere S^2 of radius 1 smoothly embedded in the Euclidean space E^3,
 Open subset S^2-{SP} of the 2-sphere S^2 of radius 1 smoothly embedded in the Euclidean space E^3}
sage: M._open_covers
[[2-sphere S^2 of radius 1 smoothly embedded in the Euclidean space E^3],
 [Open subset S^2-{NP} of the 2-sphere S^2 of radius 1 smoothly embedded in the Euclidean space E^3,
  Open subset S^2-{SP} of the 2-sphere S^2 of radius 1 smoothly embedded in the Euclidean space E^3]]

Actually the parallelizable parts are declared on the fly, in the same spirit as the charts and other structures on the manifod. Even the parallelizable aspect of the manifold can appear quite late after the manifold instanciation. For example, in this notebook about S^1, the manifold is (implicitly) declared parallelizable only in cell no. 21, by declaring a vector frame on it. This action creates the object X(M) as a VectorFieldFreeModule. Previously, only the modules X(A) and X(B) had been instanciated. However, one could imagine to add the keyword argument parallelizable to the manifold constructor (with default being False).

I guess a subtlety is that in sage.manifolds a manifold M is thought to be immutable but new submanifolds on it can be declared imperatively. At any point, of course only finitely many submanifolds have been declared in M as a Python object, but it's not clear that M as a mathematical object is really assumed to be covered by only a finite number of parallelizable submanifolds...

You're right, this should be stated in the manifold documentation. The whole idea is described in Sec. 3.2 and 3.3 of this document.
Note that the finite number of parallelizable pieces is not a very severe limitation. For instance this holds for any compact manifold. Actually I don't know any manifold that is not of this type, not that I doubt that some examples can be constructed...

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 29, 2022

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

d5e715esage.tensor.modules.reflexive_module: New, refactor finite rank free modules and vector field modules through the new classes

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 29, 2022

Changed commit from 7194893 to d5e715e

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 29, 2022

comment:19

Here's a draft of refactoring along these lines. (Of course src/sage/tensor/modules/reflexive_module.py would still need more documentation.)

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 3, 2022

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

2a7b445src/doc/en/reference/tensor_free_modules/tensors.rst: Add reflexive_module

@egourgoulhon
Copy link
Member

comment:31

I have two questions regarding the inheritance structure:

1/ There is a disymmetry in the MRO between FiniteRankFreeModule, FiniteDualFreeModule and TensorFreeModule: for the first two classes, the inheritance order is (ReflexiveModule_(base or dual), FiniteRankFreeModule_abstract), while for the third class it is the opposite order: (FiniteRankFreeModule_abstract, ReflexiveModule_tensor). Is this intended?

2/ The class VectorFieldModule_abstract has no method at all. Is such a class necessary? The only use I can see is that classes that inherit from it inherit from the pair (UniqueRepresentation, ReflexiveModule_abstract), but it would be more clear to make this explicit in those classes, instead of using VectorFieldModule_abstract as a proxy for (UniqueRepresentation, ReflexiveModule_abstract). Also, the name VectorFieldModule_abstract could be misleading since for instance DiffFormModule inherits from it.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 4, 2022

Changed commit from 2a7b445 to 05daeee

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 4, 2022

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

05daeeesrc/sage/tensor/modules/tensor_free_module.py (TensorFreeModule): Make MRO consistent with FiniteRankFreeModule

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 4, 2022

comment:33
  1. Thanks for catching this, fixed.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 4, 2022

comment:34
  1. I guess I was planning to move some methods from VectorFieldModule to VectorField_abstract. Candidates would be domain, ambient_domain, and perhaps some others. But I agree that the name VectorFieldModule_abstract is misleading... Would you have a suggestion for a better name? It also doesn't have to happen on this ticket -- I can just back it out.

@egourgoulhon
Copy link
Member

comment:35

Replying to Matthias Köppe:

  1. I guess I was planning to move some methods from VectorFieldModule to VectorField_abstract. Candidates would be domain, ambient_domain, and perhaps some others. But I agree that the name VectorFieldModule_abstract is misleading... Would you have a suggestion for a better name?

Maybe FieldModule_abstract?

It also doesn't have to happen on this ticket -- I can just back it out.

OK.

@egourgoulhon
Copy link
Member

comment:36

May I ask what do we choose? Renaming VectorFieldModule_abstract to FieldModule_abstract (or something similar) or backing it out for this ticket?

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 12, 2022

comment:37

I'll back it out

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 12, 2022

Changed commit from 05daeee to ebc2f27

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 12, 2022

Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:

610545eTensorFreeModule.tensor_factors: Go through methods tensor_type, base_module
a6ba101TensorFieldModule.tensor_factors: New
8e0bf36TensorFieldModule: Put it in the category Modules.TensorProducts
20bd0d3src/sage/manifolds/differentiable/tensorfield_module.py: Update copyright according to git blame -w --date=format:%Y FILE | sort -k2
38e6b3esage.tensor.modules.reflexive_module: New, refactor finite rank free modules and vector field modules through the new classes
04a4daasrc/sage/tensor/modules/reflexive_module.py: Make all classes subclasses of ..._abstract
53e944asrc/sage/tensor/modules/reflexive_module.py: Add documentation
f107a77src/doc/en/reference/tensor_free_modules/tensors.rst: Add reflexive_module
d2228bdsrc/sage/tensor/modules/tensor_free_module.py (TensorFreeModule): Make MRO consistent with FiniteRankFreeModule
ebc2f27WIP get rid of VectorFieldModule_abstract

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 12, 2022

Changed dependencies from #34448 to none

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 12, 2022

Changed commit from ebc2f27 to 8a2866b

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 12, 2022

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

8a2866bget rid of VectorFieldModule_abstract

@egourgoulhon
Copy link
Member

comment:42

Replying to Matthias Köppe:

I'll back it out

Thanks!

One minor last thing: as revealed by the patchbot, ReflexiveModule_abstract is imported by unused in vectorfield_module.py.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 13, 2022

Changed commit from 8a2866b to 74601e6

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 13, 2022

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

74601e6Remove unused import

@egourgoulhon
Copy link
Member

Reviewer: Eric Gourgoulhon

@egourgoulhon
Copy link
Member

comment:44

Thanks!

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 13, 2022

comment:45

Thank you!

@vbraun
Copy link
Member

vbraun commented Nov 15, 2022

Changed branch from u/mkoeppe/vectorfieldmodule_tensor_product to 74601e6

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants