Opened 4 years ago
Closed 3 years ago
#18640 closed enhancement (fixed)
Topological manifolds: scalar fields
Reported by:  egourgoulhon  Owned by:  egourgoulhon 

Priority:  major  Milestone:  sage7.2 
Component:  geometry  Keywords:  topological manifolds 
Cc:  mbejger  Merged in:  
Authors:  Eric Gourgoulhon, Michal Bejger, Travis Scrimshaw  Reviewers:  Travis Scrimshaw, Eric Gourgoulhon 
Report Upstream:  N/A  Work issues:  
Branch:  9ec7d3e (Commits)  Commit:  9ec7d3e4d636676e7091f6e7032f274853677418 
Dependencies:  #18529  Stopgaps: 
Description (last modified by )
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 K^{n} > K, where V is some chart codomain and n=dim(M)CoordFunctionSymb
: symbolic coordinate functions
MultiCoordFunction
: functions V\subset K^{n} > K^{m}, where V is some chart codomain and m some positive integerScalarFieldAlgebra
: set C^{0}(M) of scalar fields M > K as a commutative algebra over K (Parent class)ScalarField
: scalar field M > K (Element class)ExpressionNice
: a subclass ofsage.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.
Change History (48)
comment:1 Changed 4 years ago by
 Description modified (diff)
comment:2 Changed 4 years ago by
 Commit changed from e06598ef9c9e90fcc7fb88061862f476143f9bab to fd8ceef75024a2e4643406fca6330ae0356c5a4a
comment:3 Changed 4 years ago by
 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 4 years ago by
 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 4 years ago by
 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 4 years ago by
 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 4 years ago by
 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 4 years ago by
 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 4 years ago by
 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 3 years ago by
 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 3 years ago by
 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 3 years ago by
 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 3 years ago by
 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 padic 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 3 years ago by
 Description modified (diff)
 Milestone changed from sage6.8 to sage6.10
 Status changed from new to needs_review
comment:15 Changed 3 years ago by
 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_spaces18175

f8f5b93  Fixing last remaining doctests.

041a5d1  Adding padics 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 3 years ago by
 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 3 years ago by
The above commit takes into account this discussion on sagedevel and on ticket #18529: it removes UniqueRepresentation
for the class ScalarFieldAlgebra
, leaving only WithEqualityById
.
comment:18 Changed 3 years ago by
 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 3 years ago by
 Commit changed from 66f2c5ad92e5ec778aa73f299f80253c0af5c67a to 2481359e6795fb331326120adf7a0fb744c1f289
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 3 years ago by
 Commit changed from 2481359e6795fb331326120adf7a0fb744c1f289 to f69c9ee4feed0222ee451f5a4edf3018f9f7d005
comment:21 Changed 3 years ago by
 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 3 years ago by
 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 3 years ago by
 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 3 years ago by
 Commit changed from ed4aa5867c63789c4152c1fa6dfa89bef2159795 to c01048f2c51f98798ef8f86548241d69e6578d56
comment:25 Changed 3 years ago by
 Milestone changed from sage6.10 to sage7.0
comment:26 Changed 3 years ago by
 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 3 years ago by
 Commit changed from 8e17d54b31901cf64a63dd6284868e2c625da81d to 3b92a85394e8fc6f7572c58d0a6ce8b1bebacce2
comment:28 followup: ↓ 29 Changed 3 years ago by
 Milestone changed from sage7.0 to sage7.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 forCoordFunction
(and haveCoordFunction
be a subclass ofAlgebraElement
), which would allow you to not duplicate code and use the coercion framework. Actually, perhapsCoordFunction
should be a subclass ofMorphism
.
Also, I converted the methods in
CoordFunction
to@abstract_method
since these get picked up by theTestSuite
as opposed to raising aNotImplementedError
. (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
andnice_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 thinkingManifold
(a function is a Python object too, so we can attach data to it) andTopologicalManifold
. Either that, or we just have one objectManifoldOptions
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 ; followup: ↓ 30 Changed 3 years ago by
Replying to tscrim:
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 computergenerated 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 forCoordFunction
(and haveCoordFunction
be a subclass ofAlgebraElement
), which would allow you to not duplicate code and use the coercion framework. Actually, perhapsCoordFunction
should be a subclass ofMorphism
.
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 theTestSuite
as opposed to raising aNotImplementedError
.
Thanks. I didn't know about this.
(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
andnice_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) andTopologicalManifold
.
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 ; followup: ↓ 31 Changed 3 years ago by
Replying to egourgoulhon:
Replying to 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 computergenerated 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 forCoordFunction
(and haveCoordFunction
be a subclass ofAlgebraElement
), which would allow you to not duplicate code and use the coercion framework. Actually, perhapsCoordFunction
should be a subclass ofMorphism
.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) andTopologicalManifold
.
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 3 years ago by
Replying to tscrim:
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 forCoordFunction
(and haveCoordFunction
be a subclass ofAlgebraElement
), which would allow you to not duplicate code and use the coercion framework. Actually, perhapsCoordFunction
should be a subclass ofMorphism
.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 sagedevel, 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) andTopologicalManifold
.
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 followup: ↓ 33 Changed 3 years ago by
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 3 years ago by
Replying to tscrim:
Another question, should
MultiCoordFunction
be a subclass ofCoordFunction
, 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 3 years ago by
Okay, next question. Do you have a reason why jacobian
does not return a matrix?
comment:35 Changed 3 years ago by
Ah, it is because its elements are instances of CoordFunction
rather than elements of SR
. Nevermind.
comment:36 Changed 3 years ago by
 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 followup: ↓ 40 Changed 3 years ago by
 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 sagedevel 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 3 years ago by
 Reviewers set to Travis Scrimshaw
comment:39 Changed 3 years ago by
 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 3 years ago by
Replying to tscrim:
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 sagedevel 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 3 years ago by
comment:42 followup: ↓ 45 Changed 3 years ago by
 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 3 years ago by
 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 3 years ago by
 Commit changed from f3f5470059519938a13905f275341d27bb5eb298 to 9ec7d3e4d636676e7091f6e7032f274853677418
comment:45 in reply to: ↑ 42 Changed 3 years ago by
Replying to tscrim:
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 (pseudoRiemannian 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 followup: ↓ 47 Changed 3 years ago by
 Status changed from needs_review to positive_review
Great. All is good with me. Positive review.
comment:47 in reply to: ↑ 46 Changed 3 years ago by
comment:48 Changed 3 years ago by
 Branch changed from public/manifolds/top_manif_scalar_fields to 9ec7d3e4d636676e7091f6e7032f274853677418
 Resolution set to fixed
 Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. New commits:
Add doctests in ScalarField