Opened 23 months ago

Closed 7 months ago

#18843 closed enhancement (fixed)

Differentiable manifolds: vector fields and tensor fields

Reported by: egourgoulhon Owned by: egourgoulhon
Priority: major Milestone: sage-7.4
Component: geometry Keywords: differentiable manifold, tensor field, vector field, differential form
Cc: mbejger, bpillet, bpage Merged in:
Authors: Eric Gourgoulhon, Michal Bejger Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: fb7f4dd (Commits) Commit: fb7f4dd6e483f9956f49475bc16707f739f1129a
Dependencies: #15916, #18783, #20770 Stopgaps:

Description (last modified by egourgoulhon)

This ticket implements tensor fields (among which vector fields and differential forms) on differentiable manifolds. This is a follow-up of #18783 within the SageManifolds project (see the metaticket #18528 for an overview). As in #18783, the non-discrete topological field K over which the differentiable manifold is defined is generic, although in most applications, K=R or K=C.

This ticket implements the following Python classes:

1/ Parent classes:

  • VectorFieldModule: module of vector fields on a differentiable manifold
  • VectorFieldFreeModule: free module of vector fields on a parallelizable differentiable manifold
  • TensorFieldModule: module of tensor fields of a given type (k,l) on a differentiable manifold
  • TensorFieldFreeModule: free module of tensor fields of a given type (k,l) on a parallelizable differentiable manifold
  • DiffFormModule: module of differential forms of a given degree p (p-forms) on a differentiable manifold
  • DiffFormFreeModule: free module of differential forms of a given degree p (p-forms) on a parallelizable differentiable manifold
  • AutomorphismFieldGroup: general linear group of the module of vector fields on a differentiable manifold
  • AutomorphismFieldParalGroup: general linear group of the free module of vector fields on a parallelizable differentiable manifold

2/ Element classes:

  • TensorField: tensor field on a differentiable manifold
    • VectorField: vector field on a differentiable manifold
    • DiffForm: p-form on differentiable manifold
    • AutomorphismField: field of tangent-space automorphisms on a differentiable manifold
  • TensorFieldParal: tensor field on a parallelizable differentiable manifold
    • VectorFieldParal: vector field on a parallelizable differentiable manifold
    • DiffFormParal: p-form on parallelizable differentiable manifold
    • AutomorphismFieldParal: field of tangent-space automorphisms on a parallelizable differentiable manifold

3/ Other classes:

  • VectorFrame: vector frame on a differentiable manifold
    • CoordFrame: coordinate vector frame on a differentiable manifold
  • CoFrame: coframe (frame of 1-forms) on a differentiable manifold
    • CoordCoFrame: coordinate coframe on a differentiable manifold

Documentation: The reference manual is produced by sage -docbuild reference/manifolds html It can also be accessed online at http://sagemanifolds.obspm.fr/doc/18843/reference/manifolds/ More documentation (e.g. example worksheets) can be found here.

Change History (69)

comment:1 Changed 23 months ago by git

  • Commit changed from d48210c11f38f90ce656f0fa25ec550d147a1892 to b9c3bc529cdd9e7c11f5f6bd10f6e29b070b7cbc

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

b9c3bc5Start to improve documentation on tensor fields on differentiable manifolds

comment:2 Changed 23 months ago by git

  • Commit changed from b9c3bc529cdd9e7c11f5f6bd10f6e29b070b7cbc to a2e0696fc335aa07d0b2ce3f6ab54c45996e6746

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

a2e0696Add method display() to tensor components and display_comp() to tensors

comment:3 Changed 23 months ago by git

  • Commit changed from a2e0696fc335aa07d0b2ce3f6ab54c45996e6746 to cad56b1f9b3f4ad59b957d7e8b1b8512bec3ae4d

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

cad56b1Improve documentation of tensor fields

comment:4 Changed 23 months ago by git

  • Commit changed from cad56b1f9b3f4ad59b957d7e8b1b8512bec3ae4d to 794c149286b90285cd1d32cd21145b2a2e3e8d53

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

794c149Improve documentation of differential forms

comment:5 Changed 23 months ago by git

  • Commit changed from 794c149286b90285cd1d32cd21145b2a2e3e8d53 to 435fb11339a9e00b87de5ceac36ed14085c197e0

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

435fb11Improve documentation of vector frames

comment:6 Changed 23 months ago by git

  • Commit changed from 435fb11339a9e00b87de5ceac36ed14085c197e0 to 8f47f64e2eb6411bcc00693f012c63974b3d4e28

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

8f47f64More improvements in the documentation of diff. manifolds

comment:7 Changed 22 months ago by git

  • Commit changed from 8f47f64e2eb6411bcc00693f012c63974b3d4e28 to 935a0f741185e30f50ad27750987d7bac63b957f

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

935a0f7Improvements in the documentation of class DiffManifold

comment:8 Changed 22 months ago by egourgoulhon

  • Description modified (diff)

comment:9 Changed 22 months ago by git

  • Commit changed from 935a0f741185e30f50ad27750987d7bac63b957f to 2c6b711ea7c8faedef4c13804c8e8f9466576c1a

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

2c6b711Improve documentation of scalar and vector fields; add doctests

comment:10 Changed 22 months ago by git

  • Commit changed from 2c6b711ea7c8faedef4c13804c8e8f9466576c1a to eeff027998d9e9c3dfec7de22543dd5501d4f768

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

eeff027Add doctests for tensor fields

comment:11 Changed 21 months ago by git

  • Commit changed from eeff027998d9e9c3dfec7de22543dd5501d4f768 to 686cee1a746cfe3e6aecf5e4087d492c11be8839

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

686cee1Full doctest coverage.

comment:12 Changed 21 months ago by git

  • Commit changed from 686cee1a746cfe3e6aecf5e4087d492c11be8839 to fcf8ec4d07fc26e065c47d83b8187ca89c6dae5f

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

fcf8ec4Improvement in simplify_sqrt_real() and remove of verbose=True in TestSuite().run()

comment:13 Changed 21 months ago by egourgoulhon

  • Dependencies changed from #15916, #18100, #18783 to #15916, #18783

comment:14 Changed 21 months ago by egourgoulhon

  • Milestone changed from sage-6.8 to sage-6.9

comment:15 Changed 20 months ago by git

  • Commit changed from fcf8ec4d07fc26e065c47d83b8187ca89c6dae5f to 307e655897455860869ede907a4619f11e024ecd

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

307e655Major improvements in the documentation of diff. manifolds (tensor field part)

comment:16 Changed 20 months ago by git

  • Commit changed from 307e655897455860869ede907a4619f11e024ecd to dc7f7a1405bae39afe1e582f440373e0a54fb78c

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

dc7f7a1Improve TensorField.__eq__ (case with no open cover known)

comment:17 Changed 20 months ago by git

  • Commit changed from dc7f7a1405bae39afe1e582f440373e0a54fb78c to 4d6f21c8ab3f399e97806baa18ec7d2dac714952

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

57b21dfAdd doctest to set_axes_labels
e558c06Improvement in simplify_sqrt_real(); TestSuite run non-verbose
d65f654Introduce open covers on top manifolds + many improvements in the documentation
00c327dSlight reorganization of the reference manual of topological manifolds (morphisms part)
f8d3f27Merge #18725 into #18640
f2fef7bSmall improvements in the documentation of differentiable manifolds
8ab80d8Improvement in simplify_sqrt_real(); minor modif. in documentation
2f231b6Major improvements in the documentation of diff. manifolds (basic part)
f0ca2deMerge #18783 into #18725
4d6f21cMerge #18843 into #18783

comment:18 Changed 20 months ago by egourgoulhon

  • Description modified (diff)
  • Milestone changed from sage-6.9 to sage-6.10
  • Status changed from new to needs_review

comment:19 Changed 19 months ago by git

  • Commit changed from 4d6f21c8ab3f399e97806baa18ec7d2dac714952 to a527726fb1d474803c95309b70f3a1e160a1faee

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

f8f5b93Fixing last remaining doctests.
041a5d1Adding p-adics to metric spaces and some cleanup.
bfa0cdfOne last doc tweak.
d13c368Fixing doc of metric spaces.
2605c0bMerge #18529 (Topological manifolds: basics) into #18175 (Implement categories for topological...)
6dec6d5Implement topological manifolds (basics, #18529) on the new categories for manifolds (#18175)
3403978Implement top. manifolds (scalar fields, #18640) on the new manifold categories (#18175)
b0521efImplement top. manifolds (morphisms, #18725) on the new manifold categories (#18175)
f643097Implement diff. manifolds (basics, #18783) on the new manifold categories (#18175)
a527726Implement diff. manifolds (tensor fields, #18843) on the new manifold categories (#18175)

comment:20 Changed 18 months ago by git

  • Commit changed from a527726fb1d474803c95309b70f3a1e160a1faee to 0ee4c41ddf9b0759d219a7d256a91a0b3e6357c9

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

252e616Remove UniqueRepresentation, leaving only WithEqualityById, for topological manifolds and charts.
e7139abMerge branch top_manif_basics without UniqueRepresentation into top_manif_scalar_fields.
e2f192fRemove UniqueRepresentation, leaving only WithEqualityById, for scalar fields on topological manifolds
668bc26Remove UniqueRepresentation, leaving only WithEqualityById, for topological manifold homsets
6518699Introduce the attribute _field_type in class TopologicalManifold to check for real and complex manifolds.
22383e6Check for real/complex manifold performed on base_field_type() instead of RR/CC
66f2c5aChange function('f', x) to function('f')(x) to accomodate the deprecation warning introduced in #17447
a28ed04Morphisms of topological manifolds with the use of base_field_type()
f31bed1Remove UniqueRepresentation from differentiable manifolds
0ee4c41Tensor fields on differentiable manifolds without unique representation

comment:21 Changed 18 months ago by git

  • Commit changed from 0ee4c41ddf9b0759d219a7d256a91a0b3e6357c9 to e8f11ffc706221d3f5a3b0c84d262519b1fa81e3

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

e8f11ffFix pickling test in tensor field modules.

comment:22 Changed 18 months ago by git

  • Commit changed from e8f11ffc706221d3f5a3b0c84d262519b1fa81e3 to a9677872c7499a8c38044dbe8442f0821380c284

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

a967787Suppress direct call to _element_constructor_ in tensor field parent classes

comment:23 Changed 18 months ago by git

  • Commit changed from a9677872c7499a8c38044dbe8442f0821380c284 to 45beae5c3809cde39d51e72016138964557ac91b

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

d8397c1Merge branch 'public/manifolds/top_manif_basics' of trac.sagemath.org:sage into public/manifolds/top_manif_basics
0b08b11Some small tweaks as part of the review.
d3e5d4dRevert to UniqueRepresentation for topological manifolds
2481359Revert to UniqueRepresentation for ScalarFieldAlgebra; better ScalarField constructor
0cc06e8Revert to UniqueRepresentation for topological manifold homsets
322d5bdRevert to UniqueRepresentation for differentiable manifolds
45beae5Revert to UniqueRepresentation for differentiable manifolds: tensor fields

comment:24 Changed 18 months ago by git

  • Commit changed from 45beae5c3809cde39d51e72016138964557ac91b to 0d68f86562cdd80f239c3093ee32d16939e6cae2

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

85d03dcChange the argument type to structure in Manifold
5251ef0Remove method _test_pickling from class TopologicalManifoldPoint
f69c9eeFix doctest error in coord_func_symb.py due to #19312 (update to pynac-0.5.2)
7889a5dChange in simplify_sqrt_real to cope with the change of != operator induced by #19312 (Sage 6.10.beta7)
e8d2ba6Differentiable manifolds: basics with the change in symbolic expression logic induced by #19312 (Sage 6.10.beta7)
0d68f86Diff. manifolds: tensor fields, with the change in symbolic expression logic induced by #19312 (Sage 6.10.beta7)

comment:25 Changed 17 months ago by git

  • Commit changed from 0d68f86562cdd80f239c3093ee32d16939e6cae2 to ecfbf2cd37624907f4d009d3096abd2b9c2312a9

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

cdfa817Merge branch u/tscrim/top_manifolds_refactor into Sage 6.10.rc1 + public/manifolds/top_manif_basics
c5f35afMake AbstractSet inherit from UniqueRepresentation; correct doctests; start to change documentation.
3a52500Some class renaming; add more examples and doctests (full coverage)
a1ba52aReplace a!=b by not(a==b) in simplify_sqrt_real to cope with the change of logic induced by #19312 (Sage 6.10.beta7)
e2c7ab3Scalar fields on the refactored topoological manifolds
cb53417Add argument full_name to AbstractNamedObject; remove _repr_() from all parent classes; improve documentation
ce03503Scalar fields on topological manifolds: slight improvements in the documentation
bc77059Morphisms on the refactored topoological manifolds
2df7af7Refactorization of differentiable manifolds, with the mixin class DifferentiableMixin
ecfbf2cTensor fields on refactored differentiable manifolds

comment:26 Changed 17 months ago by git

  • Commit changed from ecfbf2cd37624907f4d009d3096abd2b9c2312a9 to 8b720a5db590ed511f424fafc92e349f6de2f768

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

c38ae80Suppress the (unused) argument category in OpenTopologicalSubmanifold; minor doc improvements
069baf4Merge branch 'public/manifolds/top_manif_basics' into Sage 7.0.beta2.
19caedbRevert to AbstractNamedObject without argument full_name; restore _repr_() in manifold classes
9d25aa0Scalar field algebras with AbstractNamedObject without argument full_name
ed4aa58ScalarFieldAlgebra does not longer inherit from AbstractNamedObject
f6c6a34Topological manifold morphisms with AbstractNamedObject without full_name
7204572Basics of diff. manifolds with AbstractNamedObject without full_name
8b720a5Tensor fields with AbstractNamedObject without full_name

comment:27 Changed 17 months ago by git

  • Commit changed from 8b720a5db590ed511f424fafc92e349f6de2f768 to 1f226bdceacae88627a26e24166d4bf6cf51b021

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

3cd03a4Add methods lift() and retract() to ManifoldSubset; add method __eq__() in CoordChange
984c3c2Revert to simple hierarchy for manifold classes
c01048fScalar fields with the simplified hierarchy for manifold classes
c866d6cMorphisms of topological manifolds with the simplified hierarchy for manifold classes
0e04631Basics of diff. manifolds with the simplified hierarchy for manifold classes
1f226bdTensor fields with the simplified hierarchy for manifold classes

comment:28 Changed 17 months ago by egourgoulhon

  • Milestone changed from sage-6.10 to sage-7.0

comment:29 Changed 16 months ago by git

  • Commit changed from 1f226bdceacae88627a26e24166d4bf6cf51b021 to 2fad9db0007feee5a4b0f64eb4ec64b35dee1f74

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

8e17d54Merge into the latest version of #18529; improve treatment of composite functions in ExpressionNice
f00be00Topological manifold morphisms: solved merge conflict with Sage 7.1.beta1
21b3968Basics of diff. manifolds: solve merge conflict with Sage 7.1.beta1
8ba4b91Tensor fields: solve merge conflict with Sage 7.1.beta1
2fad9dbCorrect doctest in class DiffFormParal

comment:30 Changed 13 months ago by git

  • Commit changed from 2fad9db0007feee5a4b0f64eb4ec64b35dee1f74 to 73b4a036e94099b6e2b368f007be82bba127c797

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

57b7b7bImplementing global_options for display formatting.
f11df50Set CoordFunctionSymbRing in the category of commutative algebras over SR; minor documentation changes
f3f5470Fixing a (essentially trivial) doctest failure.
7b3dab3Merge branch 'public/manifolds/top_manif_scalar_fields' of git://trac.sagemath.org/sage into Sage 7.2.rc0
9ec7d3ePython 3 format for print in manifolds
d190b37Morphisms of topological manifolds with coordinate functions as algebra elements
2577fc5Fixing a (essentially trivial) doctest failure.
7125e29Python3 format for print in morphims of topological manifolds
bd2f35aDiff. manifolds with coordinate functions as algebra elements
73b4a03Tensor fields with with coordinate functions as algebra elements

comment:31 Changed 13 months ago by egourgoulhon

  • Milestone changed from sage-7.0 to sage-7.2

comment:32 Changed 13 months ago by git

  • Commit changed from 73b4a036e94099b6e2b368f007be82bba127c797 to 29832ece0fa3114f9a936ea1ebd0825305322698

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

4fad094Merge branch 'public/manifolds/top_manif_morphisms' of trac.sagemath.org:sage into public/manifolds/top_manif_morphisms
cfecb18Reviewer changes and tweaks for continuous maps ticket.
efcb618Modify authorship for continuous maps.
84051b0Merge branch 'public/manifolds/diff_manif_basics' of trac.sagemath.org:sage into public/manifolds/diff_manif_basics
97172ddBasics of differentiable manifolds with changes in morphisms of topological manifolds
29832ecUse @cached_method for VectorFieldModule.identity_map() and AutomorphismFieldGroup.one()

comment:33 Changed 12 months ago by git

  • Commit changed from 29832ece0fa3114f9a936ea1ebd0825305322698 to 2150a43db88da8007dd6b35b1a5c6876e1c67eab

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

2150a43Correct doctests in vector fields after the merge of #20770 in sage 7.3.beta3

comment:34 Changed 12 months ago by egourgoulhon

  • Dependencies changed from #15916, #18783 to #15916, #18783, #20770
  • Milestone changed from sage-7.2 to sage-7.3

comment:35 follow-ups: Changed 11 months ago by tscrim

  • Commit changed from 2150a43db88da8007dd6b35b1a5c6876e1c67eab to 3ab0af048e9ba51f09752a02f0d6544a53ffe15e
  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to needs_work

Okay, here's the first round. Overall, the code looks okay, but I haven't check things in detail yet. I've made my way through most of the doc; there are likely some other doc fixes that I will need to do, but I've done a fair bit of clean-up. However, there are some issues:

  • To run the tests on tensorfield.py takes ~172 seconds (that is after marking a bunch of them as # long time). This is too long IMO, and we need to either find a way to speed things up or simplify the tests somehow (smaller manifolds perhaps?) so they don't take so long. One or two non-trivial top-level examples are good. For the doctests, it just needs to be small examples showing the functionality. For any more complicated examples needed to do a more complete test should either be relatively small (in terms of number of lines) and be marked as # long time or you need to have a separate file with the complete examples where most (all?) tests are marked as # long time.
  • I'm not sure about importing xder into the global namespace. exterior_derivative I am okay with since it is explicit, but xder is not a descriptive name IMO. If you want it, then you can import it in your init.sage file (in the ~/.sage folder).
  • I added some long versions of each of the names. It is good to at least have the long name. I actually changed exterior_der to exterior_derivative. I am not opposed to having an alias to xder though.
  • While in general, it is good to adhere to 80 character lines, sometimes it just makes it harder read (which is why it is a guideline). I exercised some judgment and made some lines go over this because IMO they become easier to read.
  • We will probably want to use the coercion framework, so __mul__ which handles the scalar action should become acted_upon or _rmul_ (and _lmul_).

I believe this is the biggest of the remaining SageManifolds tickets, so we just need to power through this and the rest should be easy...

comment:36 in reply to: ↑ 35 ; follow-up: Changed 11 months ago by egourgoulhon

Replying to tscrim:

Okay, here's the first round.

Thank you very much for reviewing this!

Overall, the code looks okay, but I haven't check things in detail yet. I've made my way through most of the doc; there are likely some other doc fixes that I will need to do, but I've done a fair bit of clean-up. However, there are some issues:

  • To run the tests on tensorfield.py takes ~172 seconds (that is after marking a bunch of them as # long time). This is too long IMO, and we need to either find a way to speed things up or simplify the tests somehow (smaller manifolds perhaps?) so they don't take so long. One or two non-trivial top-level examples are good. For the doctests, it just needs to be small examples showing the functionality. For any more complicated examples needed to do a more complete test should either be relatively small (in terms of number of lines) and be marked as # long time or you need to have a separate file with the complete examples where most (all?) tests are marked as # long time.

OK I will try to shorten significantly the doctests.

  • I'm not sure about importing xder into the global namespace. exterior_derivative I am okay with since it is explicit, but xder is not a descriptive name IMO. If you want it, then you can import it in your init.sage file (in the ~/.sage folder).

I agree xder is not as descriptive as exterior_derivative. Certainly the latter should be the primary function name. However for the end user manipulating a lot of differential forms, it is certainly desirable to have a short name (in addition to TAB completion), as diff is a short name for derivative. We may even think of a shorter name, like xd, as a reminder of the standard mathematical notation d.

  • I added some long versions of each of the names. It is good to at least have the long name. I actually changed exterior_der to exterior_derivative. I am not opposed to having an alias to xder though.
  • While in general, it is good to adhere to 80 character lines, sometimes it just makes it harder read (which is why it is a guideline). I exercised some judgment and made some lines go over this because IMO they become easier to read.

OK.

  • We will probably want to use the coercion framework, so __mul__ which handles the scalar action should become acted_upon or _rmul_ (and _lmul_).

Yes, this seems desirable. I'll try it.

I believe this is the biggest of the remaining SageManifolds tickets,

Yes, certainly.

so we just need to power through this and the rest should be easy...

comment:37 in reply to: ↑ 36 ; follow-up: Changed 11 months ago by tscrim

Replying to egourgoulhon:

Replying to tscrim:

Okay, here's the first round.

Thank you very much for reviewing this!

No problem. Sorry its taken so long. Hopefully I can make it through this soon.

  • I'm not sure about importing xder into the global namespace. exterior_derivative I am okay with since it is explicit, but xder is not a descriptive name IMO. If you want it, then you can import it in your init.sage file (in the ~/.sage folder).

I agree xder is not as descriptive as exterior_derivative. Certainly the latter should be the primary function name. However for the end user manipulating a lot of differential forms, it is certainly desirable to have a short name (in addition to TAB completion), as diff is a short name for derivative. We may even think of a shorter name, like xd, as a reminder of the standard mathematical notation d.

We currently have no differential function, just its shorten diff, which is the Sage equivalent for derivative. I am not opposed to having an xder alias, just importing it into the global namespace (by default) as it is not a "standard" short name. However, you could include the import of xder as part of the examples/tutorials. I do think that xder is okay as a method alias for exterior_derivative as well.

IMO xd, like d, is too short and generic to be useful at the top-level.

comment:38 in reply to: ↑ 37 ; follow-up: Changed 11 months ago by egourgoulhon

Replying to tscrim:

We currently have no differential function, just its shorten diff, which is the Sage equivalent for derivative. I am not opposed to having an xder alias, just importing it into the global namespace (by default) as it is not a "standard" short name. However, you could include the import of xder as part of the examples/tutorials.

I agree that the exterior derivative is too specialized to have it in the global namespace of any sage session. On the other side, end users working with differential forms may feel cumbersome to perform some import in each of their sessions. A solution could be to modify the top function Manifold so that it imports xder into the global namespace. Is such a thing possible/desirable?

comment:39 in reply to: ↑ 38 ; follow-up: Changed 11 months ago by tscrim

Replying to egourgoulhon:

Replying to tscrim:

We currently have no differential function, just its shorten diff, which is the Sage equivalent for derivative. I am not opposed to having an xder alias, just importing it into the global namespace (by default) as it is not a "standard" short name. However, you could include the import of xder as part of the examples/tutorials.

I agree that the exterior derivative is too specialized to have it in the global namespace of any sage session. On the other side, end users working with differential forms may feel cumbersome to perform some import in each of their sessions. A solution could be to modify the top function Manifold so that it imports xder into the global namespace. Is such a thing possible/desirable?

This is what the init.sage is for. It is called every time Sage is loaded. I feel that one import to setup an environment with a convenience function is very little to ask. IMO, it is not good for the entry point Manifold to have side effects.

comment:40 in reply to: ↑ 39 ; follow-ups: Changed 11 months ago by egourgoulhon

Replying to tscrim:

Replying to egourgoulhon:

I agree that the exterior derivative is too specialized to have it in the global namespace of any sage session. On the other side, end users working with differential forms may feel cumbersome to perform some import in each of their sessions. A solution could be to modify the top function Manifold so that it imports xder into the global namespace. Is such a thing possible/desirable?

This is what the init.sage is for. It is called every time Sage is loaded.

What about the SageMathCloud? It's certainly technically possible to define some init.sage there, but one has to open a terminal and edit a file, etc.

Also a drawback of init.sage is that your worksheets cannot easily be shared with somebody else, or made publicly available for repeating the calculation.

I feel that one import to setup an environment with a convenience function is very little to ask. IMO, it is not good for the entry point Manifold to have side effects.

What about Manifold.import_convenience_functions(verbose=True) or something like this? It could (i) put functions like xder in the global namespace and (ii) print out the list of such functions if verbose is True.

comment:41 in reply to: ↑ 40 Changed 11 months ago by egourgoulhon

Replying to egourgoulhon:

What about Manifold.import_convenience_functions(verbose=True) or something like this? It could (i) put functions like xder in the global namespace and (ii) print out the list of such functions if verbose is True.

Granted: from sage.manifolds.utilities import xder is as short to write...

comment:42 in reply to: ↑ 40 ; follow-up: Changed 11 months ago by tscrim

Replying to egourgoulhon:

Replying to tscrim:

Replying to egourgoulhon:

I agree that the exterior derivative is too specialized to have it in the global namespace of any sage session. On the other side, end users working with differential forms may feel cumbersome to perform some import in each of their sessions. A solution could be to modify the top function Manifold so that it imports xder into the global namespace. Is such a thing possible/desirable?

This is what the init.sage is for. It is called every time Sage is loaded.

What about the SageMathCloud? It's certainly technically possible to define some init.sage there, but one has to open a terminal and edit a file, etc.

Also a drawback of init.sage is that your worksheets cannot easily be shared with somebody else, or made publicly available for repeating the calculation.

For SMC worksheets, you can easily have the setup in an %auto block.

I feel that one import to setup an environment with a convenience function is very little to ask. IMO, it is not good for the entry point Manifold to have side effects.

What about Manifold.import_convenience_functions(verbose=True) or something like this? It could (i) put functions like xder in the global namespace and (ii) print out the list of such functions if verbose is True.

That is no different than doing a usual import. I guess a question that I have is how many such functions do you think there will eventually be?

comment:43 in reply to: ↑ 42 Changed 11 months ago by egourgoulhon

Replying to tscrim:

What about Manifold.import_convenience_functions(verbose=True) or something like this? It could (i) put functions like xder in the global namespace and (ii) print out the list of such functions if verbose is True.

That is no different than doing a usual import.

Yes I realized this just after posting my comment...

I guess a question that I have is how many such functions do you think there will eventually be?

Not much... So let's stay with the usual import ;-)

comment:44 Changed 11 months ago by git

  • Commit changed from 3ab0af048e9ba51f09752a02f0d6544a53ffe15e to dcb08fc969128ca01e9c5bb88ec1cd93aba334ff

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

d7ad8e1Merge branch 'public/manifolds/diff_manif_tensor_fields' of git://trac.sagemath.org/sage into sage 7.3.beta5
dcb08fcDecrease doctest times in tensor fields; remove xder from the global namespace

comment:45 in reply to: ↑ 35 Changed 11 months ago by egourgoulhon

Replying to tscrim:

  • To run the tests on tensorfield.py takes ~172 seconds (that is after marking a bunch of them as # long time). This is too long IMO, and we need to either find a way to speed things up or simplify the tests somehow (smaller manifolds perhaps?) so they don't take so long. One or two non-trivial top-level examples are good. For the doctests, it just needs to be small examples showing the functionality. For any more complicated examples needed to do a more complete test should either be relatively small (in terms of number of lines) and be marked as # long time or you need to have a separate file with the complete examples where most (all?) tests are marked as # long time.

By selecting simpler examples, the last commit decreases by a factor ~2 the doctest time of tensorfield.py. It's difficult to go below this because the manifold dimension is already at its minimum (for a non-parallelizable manifold): it is 2 for all tests.

  • I'm not sure about importing xder into the global namespace. exterior_derivative I am okay with since it is explicit, but xder is not a descriptive name IMO. If you want it, then you can import it in your init.sage file (in the ~/.sage folder).

xder is no longer in the global namespace. I've also suppressed exterior_derivative, but maybe we can restore the latter, as you suggest.

  • I added some long versions of each of the names. It is good to at least have the long name. I actually changed exterior_der to exterior_derivative. I am not opposed to having an alias to xder though.

In the same spirit, I have renamed lie_der to lie_derivative and set lie_der as an alias to it.

  • We will probably want to use the coercion framework, so __mul__ which handles the scalar action should become acted_upon or _rmul_ (and _lmul_).

Actually _rmul_ is already implemented in tensor fields and handles the scalar action. The current __mul__ handles mostly the tensor product. Since this is an external operation (w.r.t. the parent module), I am not sure that it can be changed to some single underscore method within the coercion framework. Note that we already had __mul__ for handling tensor products in sage.tensor.modules.free_module_tensor.FreeModuleTensor.

comment:46 Changed 11 months ago by egourgoulhon

  • Status changed from needs_work to needs_review

comment:47 Changed 9 months ago by bpage

I checked out this commit with 7.4 beta0. All of my Jupyter worksheets that use forms and vectors seem to work fine. I would like to encourage you to include this functionality in Sage in the next possible release. I would give this a positive review except that the commit is still based on an older release of Sage and that milestone has passed.

comment:48 Changed 9 months ago by bpage

  • Cc bpage added

comment:49 follow-up: Changed 9 months ago by tscrim

I am (still) making my way through my second pass of the code and doc, but hopefully soon I will have finished.

comment:50 in reply to: ↑ 49 Changed 9 months ago by egourgoulhon

Replying to tscrim:

I am (still) making my way through my second pass of the code and doc, but hopefully soon I will have finished.

Thanks for this update and your work on this.

Replying to bpage: good to know that all your Jupyter worksheets passed. However, we should really wait for Travis detailed review before concluding with this ticket.

Last edited 9 months ago by egourgoulhon (previous) (diff)

comment:51 Changed 8 months ago by git

  • Commit changed from dcb08fc969128ca01e9c5bb88ec1cd93aba334ff to 7cf4ff47794e551141bf475efa2accdaefda3d53

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

c58924fMerge branch 'develop' into public/manifolds/diff_manif_tensor_fields
27a369eMerge branch 'develop' into public/manifolds/diff_manif_tensor_fields
13cb850Merge branch 'develop' into public/manifolds/diff_manif_tensor_fields
88295c6Merge branch 'develop' into public/manifolds/diff_manif_tensor_fields
58f7fb9Merge branch 'develop' into public/manifolds/diff_manif_tensor_fields
3e73095Merge branch 'develop' into public/manifolds/diff_manif_tensor_fields
4955c3cFinal review of everything.
7cf4ff4Merge branch 'public/manifolds/diff_manif_tensor_fields' of trac.sagemath.org:sage into public/manifolds/diff_manif_tensor_fields

comment:52 follow-up: Changed 8 months ago by tscrim

I (finally) made it through the code again, and most of my changes were smaller documentation cleanups and fixes. I changed some custom caching into @cached_method (IIRC it is faster) and separated out the zero element into zero (with @cached_method). If you're good with my changes, then we can set this to positive review (and hopefully sneak this into 7.4).

comment:53 Changed 8 months ago by git

  • Commit changed from 7cf4ff47794e551141bf475efa2accdaefda3d53 to a841f794bb2bc29fd8af4b212e6e2a93b5c58057

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

58ae506Merge branch 'public/manifolds/diff_manif_tensor_fields' of git://trac.sagemath.org/sage into Sage 7.4.beta6
8219403Fix documentation error; change in TensorField comparison to zero
a841f79Python 3 compatible syntax in tensor fields

comment:54 in reply to: ↑ 52 Changed 8 months ago by egourgoulhon

Replying to tscrim:

I (finally) made it through the code again, and most of my changes were smaller documentation cleanups and fixes. I changed some custom caching into @cached_method (IIRC it is faster) and separated out the zero element into zero (with @cached_method).

Thank you very much for your review and the changes in the code!

I've committed a few minor changes: besides fixing some documentation error (missing blank line),

  • this reverts to the previous version of comparison to zero in TensorField.__eq__: indeed the following code in commit 7cf4ff4
     if other == 0:
         return self.is_zero()
     elif other in ZZ:
         return False
    
    could return True for a == b with a being a zero vector, say, and b being a zero 2-form. We do not want this behavior, do we? We should only allow for the possibility to write a == 0 as a shortcut for a.is_zero().
  • this changes some syntax to make the code compatible with Python 3 (in view of the future migration): all the .itervalues() and .iteritems() have been replaced by .values() and .items() respectively. I've not used the functions itervalues and iteritems from six, since all dictionaries are small and changing from an iterator to a list (in the Python 2.7 version) should not make a big difference (for instance, I've checked that there is no change in the doctest time).

If you're good with my changes, then we can set this to positive review (and hopefully sneak this into 7.4).

It's OK for me ;-)

comment:55 follow-up: Changed 8 months ago by tscrim

I'm okay with your changes modulo the straight revert to this isinstance(other, (int, Integer)). What if other = QQ.zero()? A better check (if slightly more in terms of time) is other in ZZ:

sage: vector([0,0,0]) == 0
True
sage: vector([0,0,0]) in ZZ
False

Although in principle, the comparison a == b should convert to a common parent...

comment:56 Changed 8 months ago by git

  • Commit changed from a841f794bb2bc29fd8af4b212e6e2a93b5c58057 to 5a5f4008bf54a7b6e71c71d13ba7aa10db7e75e6

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

5a5f400Slight change in TensorField.__eq__ (comparison to zero)

comment:57 in reply to: ↑ 55 Changed 8 months ago by egourgoulhon

Replying to tscrim:

A better check (if slightly more in terms of time) is other in ZZ:

OK, I've implemented it in the last commit.

comment:58 follow-up: Changed 8 months ago by tscrim

  • Milestone changed from sage-7.3 to sage-7.4
  • Status changed from needs_review to positive_review

Thanks.

comment:59 in reply to: ↑ 58 Changed 8 months ago by egourgoulhon

Thank you for the detailed review work and the resulting enhancement of the code!

comment:60 follow-up: Changed 8 months ago by vbraun

  • Status changed from positive_review to needs_work
[dochtml] OSError: [manifolds] /mnt/disk/home/release/Sage/local/lib/python2.7/site-packages/sage/manifolds/chart.py:docstring of sage.manifolds.chart:21: WARNING: citation not found: Lee13

comment:61 Changed 8 months ago by git

  • Commit changed from 5a5f4008bf54a7b6e71c71d13ba7aa10db7e75e6 to 8860387a6637d8c0d117c8fcfcf1524fcee6ff1c

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

8860387Add missing reference Lee13

comment:62 in reply to: ↑ 60 Changed 8 months ago by egourgoulhon

  • Status changed from needs_work to needs_review

Replying to vbraun:

[dochtml] OSError: [manifolds] /mnt/disk/home/release/Sage/local/lib/python2.7/site-packages/sage/manifolds/chart.py:docstring of sage.manifolds.chart:21: WARNING: citation not found: Lee13

Oups! Sorry about this. The last commit adds the missing reference.

comment:63 Changed 8 months ago by egourgoulhon

  • Status changed from needs_review to positive_review

Having checked that, after a make doc-clean && make doc, the issue is fixed, I am taking the liberty of setting the ticket back to posivite review.

comment:64 follow-up: Changed 7 months ago by vbraun

  • Status changed from positive_review to needs_work

Conflicts with #21454

comment:65 Changed 7 months ago by git

  • Commit changed from 8860387a6637d8c0d117c8fcfcf1524fcee6ff1c to fb7f4dd6e483f9956f49475bc16707f739f1129a

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

fb7f4ddBibliographic references for tensor fields moved to the master file

comment:66 in reply to: ↑ 64 Changed 7 months ago by egourgoulhon

  • Status changed from needs_work to needs_review

Replying to vbraun:

Conflicts with #21454

Fixed!

comment:67 follow-up: Changed 7 months ago by tscrim

  • Status changed from needs_review to positive_review

comment:68 in reply to: ↑ 67 Changed 7 months ago by egourgoulhon

Replying to tscrim:

Thanks!

comment:69 Changed 7 months ago by vbraun

  • Branch changed from public/manifolds/diff_manif_tensor_fields to fb7f4dd6e483f9956f49475bc16707f739f1129a
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.