Opened 7 years ago

Closed 6 years ago

Topological manifolds: scalar fields

Reported by: Owned by: egourgoulhon egourgoulhon major sage-7.2 geometry topological manifolds mbejger Eric Gourgoulhon, Michal Bejger, Travis Scrimshaw Travis Scrimshaw, Eric Gourgoulhon N/A 9ec7d3e 9ec7d3e4d636676e7091f6e7032f274853677418 #18529

This ticket implements scalar fields on topological manifolds. This is a follow up of ticket #18529 within the SageManifolds project. See the metaticket #18528 for an overview. By scalar field, it is meant a continuous map f: M --> K, where K is a topological field and M a topological manifold over K.

This ticket implements the following Python classes:

• `CoordFunction`: abstract base class for coordinate functions, i.e. functions V\subset Kn --> K, where V is some chart codomain and n=dim(M)
• `CoordFunctionSymb`: symbolic coordinate functions
• `MultiCoordFunction`: functions V\subset Kn --> Km, where V is some chart codomain and m some positive integer
• `ScalarFieldAlgebra`: set C0(M) of scalar fields M --> K as a commutative algebra over K (Parent class)
• `ScalarField`: scalar field M --> K (Element class)
• `ExpressionNice`: a subclass of `sage.symbolic.expression.Expression` with enhanced display of callable symbolic expressions

Internally, `ScalarField`'s are described by their coordinate representations in various charts, which are implemented as a dictionary of `CoordFunction`'s, with the charts as keys. At the moment, there is only one concrete class for coordinate functions: `CoordFunctionSymb` (functions described by symbolic expressions of the coordinates), but in the future there should be numerical coordinate functions (hence the abstract base class `CoordFunction`).

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

comment:1 Changed 7 years ago by egourgoulhon

• Description modified (diff)

comment:2 Changed 7 years ago by git

• Commit changed from e06598ef9c9e90fcc7fb88061862f476143f9bab to fd8ceef75024a2e4643406fca6330ae0356c5a4a

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

 ​fd8ceef `Add doctests in ScalarField`

comment:3 Changed 7 years ago by git

• Commit changed from fd8ceef75024a2e4643406fca6330ae0356c5a4a to 0e2c5fa2e64f4ea9bee2bf97e7afa76fa0c977f8

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

 ​0e2c5fa `Add doctests in ScalarFieldAlgebra and CoordFunction`

comment:4 Changed 7 years ago by git

• Commit changed from 0e2c5fa2e64f4ea9bee2bf97e7afa76fa0c977f8 to ec1fcc711b4a4407d86d02358e81f311e5b58236

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

 ​ec1fcc7 `More doctests in sage/manifolds/utilities.py`

comment:5 Changed 7 years ago by git

• Commit changed from ec1fcc711b4a4407d86d02358e81f311e5b58236 to 77416edb0f78a2c96c36476656bd56eb81c95f43

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

 ​77416ed `Reorganization of documentation of ExpressionNice.`

comment:6 Changed 7 years ago by git

• Commit changed from 77416edb0f78a2c96c36476656bd56eb81c95f43 to ccc2f813b93348192a6ec16a069c0e263da5043f

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

 ​ccc2f81 `Remove unnecessary import in chart.py`

comment:7 Changed 7 years ago by git

• Commit changed from ccc2f813b93348192a6ec16a069c0e263da5043f to aad7c589be80e33477da3c77cfaace209827fcd0

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

 ​aad7c58 `Modifications in CoordChange and TopManifold.__init__ to allow for subclasses.`

comment:8 Changed 7 years ago by git

• Commit changed from aad7c589be80e33477da3c77cfaace209827fcd0 to 3f22784304d9cb92f1c86df61ef57398631c8a13

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

 ​3f22784 `Minor modifications in CoordFunctionSymb`

comment:9 Changed 7 years ago by git

• Commit changed from 3f22784304d9cb92f1c86df61ef57398631c8a13 to ba5d142ab94093638d2769d9cf5cb4fc2b3d16ac

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

 ​ba5d142 `Improvement in simplify_sqrt_real() and in the documentation.`

comment:10 Changed 7 years ago by git

• Commit changed from ba5d142ab94093638d2769d9cf5cb4fc2b3d16ac to 9977194c487d29f8796b27d189260d384eaa80e4

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

 ​9977194 `Minor improvements in the doc of top manifolds (scalar fields part)`

comment:11 Changed 7 years ago by git

• Commit changed from 9977194c487d29f8796b27d189260d384eaa80e4 to 63357e44951ca82a2dccad7657c39189eaeb4d17

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

 ​63357e4 `Introduce open covers on top manifolds and improve documentation`

comment:12 Changed 7 years ago by git

• Commit changed from 63357e44951ca82a2dccad7657c39189eaeb4d17 to 92da04bba951ac139bce54c1c8cf49a353d15b46

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

 ​92da04b `Slight reorganization of the reference manual for topological manifolds (scalar field part)`

comment:13 Changed 7 years ago by git

• Commit changed from 92da04bba951ac139bce54c1c8cf49a353d15b46 to 31d3fa59345c6140f1b4fb56804a2119ce35f8dd

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

 ​7809ebf `Minor modifications in classes TopManifold and Chart.` ​99cc8c1 `Introduce Chart._init_coordinates() to simplify constructors of Chart and RealChart` ​26fc318 `Modifications in CoordChange to allow for subclasses.` ​a74c2e0 `Add example of p-adic manifold` ​542b82a `Suppress verbose in TestSuite().run; minor improvements in documentation` ​26c4890 `Minor improvements in the doc of topological manifolds (basics part)` ​be3ff74 `Introduce open covers on top manifolds` ​4de19a7 `Merge branch 'public/manifolds/top_manif_basics' of git://trac.sagemath.org/sage into sage 6.9` ​31d3fa5 `Merge #18640 into #18529`

comment:14 Changed 7 years ago by egourgoulhon

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

comment:15 Changed 7 years ago by git

• Commit changed from 31d3fa59345c6140f1b4fb56804a2119ce35f8dd to 34039787da7648cb0440a7a788a4a5b8d8d25250

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

 ​f810aa6 `Reworking the category of manifolds.` ​375ff46 `Fixing doctest failures and letting a few other rings know they are metric spaces.` ​8b851a0 `Merge branch 'develop' into public/categories/topological_metric_spaces-18175` ​f8f5b93 `Fixing last remaining doctests.` ​041a5d1 `Adding p-adics to metric spaces and some cleanup.` ​bfa0cdf `One last doc tweak.` ​d13c368 `Fixing doc of metric spaces.` ​2605c0b `Merge #18529 (Topological manifolds: basics) into #18175 (Implement categories for topological...)` ​6dec6d5 `Implement topological manifolds (basics, #18529) on the new categories for manifolds (#18175)` ​3403978 `Implement top. manifolds (scalar fields, #18640) on the new manifold categories (#18175)`

comment:16 Changed 7 years ago by git

• Commit changed from 34039787da7648cb0440a7a788a4a5b8d8d25250 to e2f192f483d51548cc4af460593172e1375a703d

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

 ​f582241 `Introduce function Manifold() as the global entry point to construct any type of manifold.` ​f342e03 `Remove UniqueRepresentation from topological manifolds, subsets and charts.` ​902908b `Revert to UniqueRepresentation for topological manifolds and charts, with the possibility to reuse manifold names.` ​252e616 `Remove UniqueRepresentation, leaving only WithEqualityById, for topological manifolds and charts.` ​e7139ab `Merge branch top_manif_basics without UniqueRepresentation into top_manif_scalar_fields.` ​e2f192f `Remove UniqueRepresentation, leaving only WithEqualityById, for scalar fields on topological manifolds`

comment:17 Changed 7 years ago by egourgoulhon

The above commit takes into account this discussion on sage-devel and on ticket #18529: it removes `UniqueRepresentation` for the class `ScalarFieldAlgebra`, leaving only `WithEqualityById`.

comment:18 Changed 7 years ago by git

• Commit changed from e2f192f483d51548cc4af460593172e1375a703d to 66f2c5ad92e5ec778aa73f299f80253c0af5c67a

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

 ​6518699 `Introduce the attribute _field_type in class TopologicalManifold to check for real and complex manifolds.` ​22383e6 `Check for real/complex manifold performed on base_field_type() instead of RR/CC` ​66f2c5a `Change function('f', x) to function('f')(x) to accomodate the deprecation warning introduced in #17447`

comment:19 Changed 7 years ago by git

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

 ​d8397c1 `Merge branch 'public/manifolds/top_manif_basics' of trac.sagemath.org:sage into public/manifolds/top_manif_basics` ​0b08b11 `Some small tweaks as part of the review.` ​d3e5d4d `Revert to UniqueRepresentation for topological manifolds` ​2481359 `Revert to UniqueRepresentation for ScalarFieldAlgebra; better ScalarField constructor`

comment:20 Changed 7 years ago by git

• Commit changed from 2481359e6795fb331326120adf7a0fb744c1f289 to f69c9ee4feed0222ee451f5a4edf3018f9f7d005

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

 ​85d03dc `Change the argument type to structure in Manifold` ​5251ef0 `Remove method _test_pickling from class TopologicalManifoldPoint` ​f69c9ee `Fix doctest error in coord_func_symb.py due to #19312 (update to pynac-0.5.2)`

comment:21 Changed 7 years ago by git

• Commit changed from f69c9ee4feed0222ee451f5a4edf3018f9f7d005 to a1ba52a0cb17df7bc45471f7f5491798d485823a

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

 ​a1ba52a `Replace a!=b by not(a==b) in simplify_sqrt_real to cope with the change of logic induced by #19312 (Sage 6.10.beta7)`

comment:22 Changed 7 years ago by git

• Commit changed from a1ba52a0cb17df7bc45471f7f5491798d485823a to ce0350398939873c5273bac486dc858e42d9b497

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

 ​2ca24ff `Merge branch 'public/manifolds/top_manif_basics' of git://trac.sagemath.org/sage into Sage 6.10.rc1` ​0fb39df `Refactoring the code to separate out the structural part of the manifold.` ​cdfa817 `Merge branch u/tscrim/top_manifolds_refactor into Sage 6.10.rc1 + public/manifolds/top_manif_basics` ​c5f35af `Make AbstractSet inherit from UniqueRepresentation; correct doctests; start to change documentation.` ​3a52500 `Some class renaming; add more examples and doctests (full coverage)` ​e2c7ab3 `Scalar fields on the refactored topoological manifolds` ​cb53417 `Add argument full_name to AbstractNamedObject; remove _repr_() from all parent classes; improve documentation` ​ce03503 `Scalar fields on topological manifolds: slight improvements in the documentation`

comment:23 Changed 7 years ago by git

• Commit changed from ce0350398939873c5273bac486dc858e42d9b497 to ed4aa5867c63789c4152c1fa6dfa89bef2159795

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

 ​c38ae80 `Suppress the (unused) argument category in OpenTopologicalSubmanifold; minor doc improvements` ​069baf4 `Merge branch 'public/manifolds/top_manif_basics' into Sage 7.0.beta2.` ​19caedb `Revert to AbstractNamedObject without argument full_name; restore _repr_() in manifold classes` ​9d25aa0 `Scalar field algebras with AbstractNamedObject without argument full_name` ​ed4aa58 `ScalarFieldAlgebra does not longer inherit from AbstractNamedObject`

comment:24 Changed 7 years ago by git

• Commit changed from ed4aa5867c63789c4152c1fa6dfa89bef2159795 to c01048f2c51f98798ef8f86548241d69e6578d56

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

 ​3cd03a4 `Add methods lift() and retract() to ManifoldSubset; add method __eq__() in CoordChange` ​984c3c2 `Revert to simple hierarchy for manifold classes` ​c01048f `Scalar fields with the simplified hierarchy for manifold classes`

comment:25 Changed 7 years ago by egourgoulhon

• Milestone changed from sage-6.10 to sage-7.0

comment:26 Changed 7 years ago by git

• Commit changed from c01048f2c51f98798ef8f86548241d69e6578d56 to 8e17d54b31901cf64a63dd6284868e2c625da81d

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

 ​af727a8 `Merge branch 'public/manifolds/top_manif_basics' of trac.sagemath.org:sage into public/manifolds/top_manif_basics` ​1113442 `Some final reviewer changes and tweaks.` ​4ed37ce `Some documentation rewording and other small tweaks.` ​cf5e98b `Use standard copyright template.` ​0c9d7fa `Merge branch 'public/manifolds/top_manif_basics' into Sage 7.1.beta0.` ​00d265c `Set verbose=False as the default in CoordChange.set_inverse; change "coord" to "coordinates" in names of ManifoldPoint methods.` ​8e17d54 `Merge into the latest version of #18529; improve treatment of composite functions in ExpressionNice`

comment:27 Changed 6 years ago by git

• Commit changed from 8e17d54b31901cf64a63dd6284868e2c625da81d to 3b92a85394e8fc6f7572c58d0a6ce8b1bebacce2

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

 ​c767696 `Merge branch 'public/manifolds/top_manif_scalar_fields' of trac.sagemath.org:sage into public/manifolds/top_manif_scalar_fields` ​3b92a85 `Some formatting and small reviewer changes.`

comment:28 follow-up: ↓ 29 Changed 6 years ago by tscrim

• Milestone changed from sage-7.0 to sage-7.2
• Status changed from needs_review to needs_info

Yay, finally had time to go through this. So, compared to the previous ticket, there is a lot less to discuss. I've mostly pushed a bunch of documentation changes on my last commit. However, I think there are still a few things to do.

• I am guessing there will eventually be another subclass of `CoordFunction`? At present, there does not seem to be a reason to have an ABC. Moreover, you could probably make any numerical coordinate chart be `._express` with the appropriate methods. However, I am wondering if there should be a parent for `CoordFunction` (and have `CoordFunction` be a subclass of `AlgebraElement`), which would allow you to not duplicate code and use the coercion framework. Actually, perhaps `CoordFunction` should be a subclass of `Morphism`.

Also, I converted the methods in `CoordFunction` to `@abstract_method` since these get picked up by the `TestSuite` as opposed to raising a `NotImplementedError`. (I haven't fixed the doctests yet in case the ABC gets removed.)

• I understand why you want to put "the" in the string representations (e.g., in `ScalarFieldAlgebra`) because it is how we would say it (in English), but this breaks convention with other parts of Sage. Moreover, it implies a certain amount of uniqueness to the manifold. However, there could be 2 different manifolds with the same dimension, name, etc.
• Similarly, in the docstrings, we should change "the *" to "this *" (sorry I didn't catch this previously).
• I don't like `omit_function_args` and `nice_derivatives` being included in the global namespace. I think these are better converted to global options. The question becomes what to attach the global options to. I am thinking `Manifold` (a function is a Python object too, so we can attach data to it) and `TopologicalManifold`. Either that, or we just have one object `ManifoldOptions` into the global namespace (but I am less favorable to this option).
• There are failing doctests in `utilities.py` likely due to recent changes in the symbolic function code (independent of my changes).

There's still likely a few more things that might need a little bit of attention, but I am pretty sure these are all of the main points.

comment:29 in reply to: ↑ 28 ; follow-up: ↓ 30 Changed 6 years ago by egourgoulhon

Yay, finally had time to go through this. So, compared to the previous ticket, there is a lot less to discuss. I've mostly pushed a bunch of documentation changes on my last commit.

Thank you very much for working on this !

However, I think there are still a few things to do.

• I am guessing there will eventually be another subclass of `CoordFunction`? At present, there does not seem to be a reason to have an ABC.

Yes there will be other subclasses of `CoordFunction`: a major project is to have a subclass for numerical functions, in order to deal with numerical manifolds (for instance computer-generated spacetimes on a grid). This was the very purpose for creating this ABC: it provides the unique interface with the rest of tensor calculus on manifolds, factoring out what has to be implemented by a concrete class (the abstract methods). So I think it is important to maintain the ABC.

Moreover, you could probably make any numerical coordinate chart be `._express` with the appropriate methods. However, I am wondering if there should be a parent for `CoordFunction` (and have `CoordFunction` be a subclass of `AlgebraElement`), which would allow you to not duplicate code and use the coercion framework. Actually, perhaps `CoordFunction` should be a subclass of `Morphism`.

OK, I will give a try to this, introducing the proper algebra (no time to do it right now, though...)

Also, I converted the methods in `CoordFunction` to `@abstract_method` since these get picked up by the `TestSuite` as opposed to raising a `NotImplementedError`.

(I haven't fixed the doctests yet in case the ABC gets removed.)

• I understand why you want to put "the" in the string representations (e.g., in `ScalarFieldAlgebra`) because it is how we would say it (in English), but this breaks convention with other parts of Sage. Moreover, it implies a certain amount of uniqueness to the manifold. However, there could be 2 different manifolds with the same dimension, name, etc.

Sorry, I am not sure I understand what you mean. Can you elaborate on this?

• Similarly, in the docstrings, we should change "the *" to "this *" (sorry I didn't catch this previously).
• I don't like `omit_function_args` and `nice_derivatives` being included in the global namespace. I think these are better converted to global options.

Agreed.

The question becomes what to attach the global options to. I am thinking `Manifold` (a function is a Python object too, so we can attach data to it) and `TopologicalManifold`.

`TopologicalManifold` sounds good, since this where the charts live in the current approach.

Either that, or we just have one object `ManifoldOptions` into the global namespace (but I am less favorable to this option).

• There are failing doctests in `utilities.py` likely due to recent changes in the symbolic function code (independent of my changes).

Indeed this reflects some bug in the latest pynac (cf. #20456); fortunately it is fixed by #20475; so if the latter is merged before the current ticket, we can leave the doctests as they are.

There's still likely a few more things that might need a little bit of attention, but I am pretty sure these are all of the main points.

comment:30 in reply to: ↑ 29 ; follow-up: ↓ 31 Changed 6 years ago by tscrim

• I am guessing there will eventually be another subclass of `CoordFunction`? At present, there does not seem to be a reason to have an ABC.

Yes there will be other subclasses of `CoordFunction`: a major project is to have a subclass for numerical functions, in order to deal with numerical manifolds (for instance computer-generated spacetimes on a grid). This was the very purpose for creating this ABC: it provides the unique interface with the rest of tensor calculus on manifolds, factoring out what has to be implemented by a concrete class (the abstract methods). So I think it is important to maintain the ABC.

I guess what I am asking is how much of concrete class `CoordFunctionSym` can be lifted to the ABC?

Moreover, you could probably make any numerical coordinate chart be `._express` with the appropriate methods. However, I am wondering if there should be a parent for `CoordFunction` (and have `CoordFunction` be a subclass of `AlgebraElement`), which would allow you to not duplicate code and use the coercion framework. Actually, perhaps `CoordFunction` should be a subclass of `Morphism`.

OK, I will give a try to this, introducing the proper algebra (no time to do it right now, though...)

It should be a fairly small class; a parent that takes a chart as input. I can work on this for my next commit.

• I understand why you want to put "the" in the string representations (e.g., in `ScalarFieldAlgebra`) because it is how we would say it (in English), but this breaks convention with other parts of Sage. Moreover, it implies a certain amount of uniqueness to the manifold. However, there could be 2 different manifolds with the same dimension, name, etc.

Sorry, I am not sure I understand what you mean. Can you elaborate on this?

When you say "the manifold," you are saying there is a unique manifold. Context makes it clear exactly which manifold you're talking about (and hence unique), but for something like documentation, "this manifold" better defines what you are reference (although personally, I would just replace it by ```self```...).

The question becomes what to attach the global options to. I am thinking `Manifold` (a function is a Python object too, so we can attach data to it) and `TopologicalManifold`.

`TopologicalManifold` sounds good, since this where the charts live in the current approach.

I will do this on the next commit.

• There are failing doctests in `utilities.py` likely due to recent changes in the symbolic function code (independent of my changes).

Indeed this reflects some bug in the latest pynac (cf. #20456); fortunately it is fixed by #20475; so if the latter is merged before the current ticket, we can leave the doctests as they are.

So we will just watch what happens with #20475, which might means we need to mark them as `# known bug` and revert them in #20475.

comment:31 in reply to: ↑ 30 Changed 6 years ago by egourgoulhon

I guess what I am asking is how much of concrete class `CoordFunctionSym` can be lifted to the ABC?

Not much: I would say that `CoordFunctionSymb` is optimal: everything that could be lifted to the ABC `CoordFunction` is already there.

Moreover, you could probably make any numerical coordinate chart be `._express` with the appropriate methods. However, I am wondering if there should be a parent for `CoordFunction` (and have `CoordFunction` be a subclass of `AlgebraElement`), which would allow you to not duplicate code and use the coercion framework. Actually, perhaps `CoordFunction` should be a subclass of `Morphism`.

OK, I will give a try to this, introducing the proper algebra (no time to do it right now, though...)

It should be a fairly small class; a parent that takes a chart as input. I can work on this for my next commit.

OK thanks.

When you say "the manifold," you are saying there is a unique manifold. Context makes it clear exactly which manifold you're talking about (and hence unique), but for something like documentation, "this manifold" better defines what you are reference (although personally, I would just replace it by ```self```...).

Thanks for these explanations. As you, I would favor `self` (which is completely unambiguous), but I remember a discussion on sage-devel, the conclusion of which was that one should avoid the mention of `self` in the documentation, to make it more readable for newcomers. So let's go for "this manifold".

The question becomes what to attach the global options to. I am thinking `Manifold` (a function is a Python object too, so we can attach data to it) and `TopologicalManifold`.

`TopologicalManifold` sounds good, since this where the charts live in the current approach.

I will do this on the next commit.

OK very good.

• There are failing doctests in `utilities.py` likely due to recent changes in the symbolic function code (independent of my changes).

Indeed this reflects some bug in the latest pynac (cf. #20456); fortunately it is fixed by #20475; so if the latter is merged before the current ticket, we can leave the doctests as they are.

So we will just watch what happens with #20475, which might means we need to mark them as `# known bug` and revert them in #20475.

OK.

comment:32 follow-up: ↓ 33 Changed 6 years ago by tscrim

Another question, should `MultiCoordFunction` be a subclass of `CoordFunction`, should they have a common ABC, or should they be unrelated (as they are now)?

comment:33 in reply to: ↑ 32 Changed 6 years ago by egourgoulhon

Another question, should `MultiCoordFunction` be a subclass of `CoordFunction`, should they have a common ABC, or should they be unrelated (as they are now)?

It seems to me that they should be unrelated, as far as inheritance is concerned. Basically a `MultiCoordFunction` object is a tuple of `CoordFunction` objects.

comment:34 Changed 6 years ago by tscrim

Okay, next question. Do you have a reason why `jacobian` does not return a matrix?

comment:35 Changed 6 years ago by tscrim

Ah, it is because its elements are instances of `CoordFunction` rather than elements of `SR`. Nevermind.

comment:36 Changed 6 years ago by git

• Commit changed from 3b92a85394e8fc6f7572c58d0a6ce8b1bebacce2 to 57b7b7b90110d1e4cc6d7e612f43cab49efc80c8

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

 ​e44f7d9 `Merge branch 'develop' into public/manifolds/top_manif_scalar_fields` ​0fd44d6 `Some cleanup and creating a parent for coordinate functions.` ​33abe9e `Marking known bugs in utilities.py.` ​57b7b7b `Implementing global_options for display formatting.`

comment:37 follow-up: ↓ 40 Changed 6 years ago by tscrim

• Status changed from needs_info to needs_review

Wee, finally had time to get it all done. I started to use ```self``` because I recall less consensus on the sage-devel thread; a good chuck of code uses it; if we add more code, it becomes standard; and it simplifies a number of the doc sentences' structure. I did not implement as much coercion as might be possible (i.e., coordinate functions could coerce to the smallest chart they are defined on), but the door is open to do so. I also added the global options and marked the tests in `utilities.py` as `# known bug`. So if my changes are good with you, then we can set this to a positive review.

comment:38 Changed 6 years ago by tscrim

• Reviewers set to Travis Scrimshaw

comment:39 Changed 6 years ago by git

• Commit changed from 57b7b7b90110d1e4cc6d7e612f43cab49efc80c8 to f11df50d2cd87172375db7d3c88e420ebb09d721

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

 ​f11df50 `Set CoordFunctionSymbRing in the category of commutative algebras over SR; minor documentation changes`

comment:40 in reply to: ↑ 37 Changed 6 years ago by egourgoulhon

Wee, finally had time to get it all done.

Thanks a lot for your work. It's nice to have the coordinate functions in the parent/element scheme! In the above commit, I have simply changed the category of the parent to `CommutativeAlgebras(SR)` and suppressed the alias `is_commutative`. I have also added a few doctests and made some slight improvements in the documentation.

I started to use ```self``` because I recall less consensus on the sage-devel thread; a good chuck of code uses it; if we add more code, it becomes standard; and it simplifies a number of the doc sentences' structure.

OK

I did not implement as much coercion as might be possible (i.e., coordinate functions could coerce to the smallest chart they are defined on), but the door is open to do so.

Indeed.

I also added the global options and marked the tests in `utilities.py` as `# known bug`.

Fine.

So if my changes are good with you, then we can set this to a positive review.

Please wait until tomorrow: I will check that the changes propagate nicely to all tickets of #18528 (I've done it already for the next ticket (#18725).

comment:41 Changed 6 years ago by egourgoulhon

• Authors changed from Eric Gourgoulhon, Michal Bejger to Eric Gourgoulhon, Michal Bejger, Travis Scrimshaw

comment:42 follow-up: ↓ 45 Changed 6 years ago by tscrim

• Reviewers changed from Travis Scrimshaw to Travis Scrimshaw, Eric Gourgoulhon

Your changes look good to me. So if everything propagates up nicely. Then you can set a positive review (unfortunately it seems like this won't get into 7.2 :/).

comment:43 Changed 6 years ago by git

• Commit changed from f11df50d2cd87172375db7d3c88e420ebb09d721 to f3f5470059519938a13905f275341d27bb5eb298

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

 ​f3f5470 `Fixing a (essentially trivial) doctest failure.`

comment:44 Changed 6 years ago by git

• Commit changed from f3f5470059519938a13905f275341d27bb5eb298 to 9ec7d3e4d636676e7091f6e7032f274853677418

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

 ​7b3dab3 `Merge branch 'public/manifolds/top_manif_scalar_fields' of git://trac.sagemath.org/sage into Sage 7.2.rc0` ​9ec7d3e `Python 3 format for print in manifolds`

comment:45 in reply to: ↑ 42 Changed 6 years ago by egourgoulhon

Your changes look good to me. So if everything propagates up nicely. Then you can set a positive review (unfortunately it seems like this won't get into 7.2 :/).

Good news: I've propagated the changes to all tickets up to #19209 (pseudo-Riemannian metrics): everything works fine! (this was also the occasion to move all print instructions to Python3 syntax and to correct the bibliographic references according to #20496). So everything is OK for me.

comment:46 follow-up: ↓ 47 Changed 6 years ago by tscrim

• Status changed from needs_review to positive_review

Great. All is good with me. Positive review.