Opened 7 years ago
Closed 6 years ago
#18640 closed enhancement (fixed)
Topological manifolds: scalar fields
Reported by: | egourgoulhon | Owned by: | egourgoulhon |
---|---|---|---|
Priority: | major | Milestone: | sage-7.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, GitHub, GitLab) | 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 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 integerScalarFieldAlgebra
: 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 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 7 years ago by
- Description modified (diff)
comment:2 Changed 7 years ago by
- Commit changed from e06598ef9c9e90fcc7fb88061862f476143f9bab to fd8ceef75024a2e4643406fca6330ae0356c5a4a
comment:3 Changed 7 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 7 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 7 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 7 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 7 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 7 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 7 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 7 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 7 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 7 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 7 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 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
- 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
- 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
- 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
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
- 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
- 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 7 years ago by
- Commit changed from 2481359e6795fb331326120adf7a0fb744c1f289 to f69c9ee4feed0222ee451f5a4edf3018f9f7d005
comment:21 Changed 7 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 7 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 7 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 7 years ago by
- Commit changed from ed4aa5867c63789c4152c1fa6dfa89bef2159795 to c01048f2c51f98798ef8f86548241d69e6578d56
comment:25 Changed 7 years ago by
- Milestone changed from sage-6.10 to sage-7.0
comment:26 Changed 7 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 6 years ago by
- Commit changed from 8e17d54b31901cf64a63dd6284868e2c625da81d to 3b92a85394e8fc6f7572c58d0a6ce8b1bebacce2
comment:28 follow-up: ↓ 29 Changed 6 years ago by
- 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 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 ; follow-up: ↓ 30 Changed 6 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 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 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 ; follow-up: ↓ 31 Changed 6 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 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 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 6 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 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) 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 follow-up: ↓ 33 Changed 6 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 6 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 6 years ago by
Okay, next question. Do you have a reason why jacobian
does not return a matrix?
comment:35 Changed 6 years ago by
Ah, it is because its elements are instances of CoordFunction
rather than elements of SR
. Nevermind.
comment:36 Changed 6 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 follow-up: ↓ 40 Changed 6 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 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
- Reviewers set to Travis Scrimshaw
comment:39 Changed 6 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 6 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 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
comment:42 follow-up: ↓ 45 Changed 6 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 6 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 6 years ago by
- Commit changed from f3f5470059519938a13905f275341d27bb5eb298 to 9ec7d3e4d636676e7091f6e7032f274853677418
comment:45 in reply to: ↑ 42 Changed 6 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 (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
- Status changed from needs_review to positive_review
Great. All is good with me. Positive review.
comment:47 in reply to: ↑ 46 Changed 6 years ago by
comment:48 Changed 6 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