Opened 2 years ago

Closed 15 months 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) Commit: 9ec7d3e4d636676e7091f6e7032f274853677418
Dependencies: #18529 Stopgaps:

Description (last modified by egourgoulhon)

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.

Change History (48)

comment:1 Changed 2 years ago by egourgoulhon

  • Description modified (diff)

comment:2 Changed 2 years ago by git

  • Commit changed from e06598ef9c9e90fcc7fb88061862f476143f9bab to fd8ceef75024a2e4643406fca6330ae0356c5a4a

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

fd8ceefAdd doctests in ScalarField

comment:3 Changed 2 years ago by git

  • Commit changed from fd8ceef75024a2e4643406fca6330ae0356c5a4a to 0e2c5fa2e64f4ea9bee2bf97e7afa76fa0c977f8

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

0e2c5faAdd doctests in ScalarFieldAlgebra and CoordFunction

comment:4 Changed 2 years ago by git

  • Commit changed from 0e2c5fa2e64f4ea9bee2bf97e7afa76fa0c977f8 to ec1fcc711b4a4407d86d02358e81f311e5b58236

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

ec1fcc7More doctests in sage/manifolds/utilities.py

comment:5 Changed 2 years ago by git

  • Commit changed from ec1fcc711b4a4407d86d02358e81f311e5b58236 to 77416edb0f78a2c96c36476656bd56eb81c95f43

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

77416edReorganization of documentation of ExpressionNice.

comment:6 Changed 2 years ago by git

  • Commit changed from 77416edb0f78a2c96c36476656bd56eb81c95f43 to ccc2f813b93348192a6ec16a069c0e263da5043f

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

ccc2f81Remove unnecessary import in chart.py

comment:7 Changed 2 years ago by git

  • Commit changed from ccc2f813b93348192a6ec16a069c0e263da5043f to aad7c589be80e33477da3c77cfaace209827fcd0

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

aad7c58Modifications in CoordChange and TopManifold.__init__ to allow for subclasses.

comment:8 Changed 2 years ago by git

  • Commit changed from aad7c589be80e33477da3c77cfaace209827fcd0 to 3f22784304d9cb92f1c86df61ef57398631c8a13

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

3f22784Minor modifications in CoordFunctionSymb

comment:9 Changed 2 years ago by git

  • Commit changed from 3f22784304d9cb92f1c86df61ef57398631c8a13 to ba5d142ab94093638d2769d9cf5cb4fc2b3d16ac

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

ba5d142Improvement in simplify_sqrt_real() and in the documentation.

comment:10 Changed 23 months ago by git

  • Commit changed from ba5d142ab94093638d2769d9cf5cb4fc2b3d16ac to 9977194c487d29f8796b27d189260d384eaa80e4

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

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

comment:11 Changed 22 months ago by git

  • Commit changed from 9977194c487d29f8796b27d189260d384eaa80e4 to 63357e44951ca82a2dccad7657c39189eaeb4d17

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

63357e4Introduce open covers on top manifolds and improve documentation

comment:12 Changed 22 months ago by git

  • Commit changed from 63357e44951ca82a2dccad7657c39189eaeb4d17 to 92da04bba951ac139bce54c1c8cf49a353d15b46

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

92da04bSlight reorganization of the reference manual for topological manifolds (scalar field part)

comment:13 Changed 22 months ago by git

  • Commit changed from 92da04bba951ac139bce54c1c8cf49a353d15b46 to 31d3fa59345c6140f1b4fb56804a2119ce35f8dd

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

7809ebfMinor modifications in classes TopManifold and Chart.
99cc8c1Introduce Chart._init_coordinates() to simplify constructors of Chart and RealChart
26fc318Modifications in CoordChange to allow for subclasses.
a74c2e0Add example of p-adic manifold
542b82aSuppress verbose in TestSuite().run; minor improvements in documentation
26c4890Minor improvements in the doc of topological manifolds (basics part)
be3ff74Introduce open covers on top manifolds
4de19a7Merge branch 'public/manifolds/top_manif_basics' of git://trac.sagemath.org/sage into sage 6.9
31d3fa5Merge #18640 into #18529

comment:14 Changed 22 months 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 22 months ago by git

  • Commit changed from 31d3fa59345c6140f1b4fb56804a2119ce35f8dd to 34039787da7648cb0440a7a788a4a5b8d8d25250

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

f810aa6Reworking the category of manifolds.
375ff46Fixing doctest failures and letting a few other rings know they are metric spaces.
8b851a0Merge branch 'develop' into public/categories/topological_metric_spaces-18175
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)

comment:16 Changed 22 months ago by git

  • Commit changed from 34039787da7648cb0440a7a788a4a5b8d8d25250 to e2f192f483d51548cc4af460593172e1375a703d

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

f582241Introduce function Manifold() as the global entry point to construct any type of manifold.
f342e03Remove UniqueRepresentation from topological manifolds, subsets and charts.
902908bRevert to UniqueRepresentation for topological manifolds and charts, with the possibility to reuse manifold names.
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

comment:17 Changed 22 months 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 21 months ago by git

  • Commit changed from e2f192f483d51548cc4af460593172e1375a703d to 66f2c5ad92e5ec778aa73f299f80253c0af5c67a

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

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

comment:19 Changed 21 months ago by git

  • Commit changed from 66f2c5ad92e5ec778aa73f299f80253c0af5c67a to 2481359e6795fb331326120adf7a0fb744c1f289

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

comment:20 Changed 21 months ago by git

  • Commit changed from 2481359e6795fb331326120adf7a0fb744c1f289 to f69c9ee4feed0222ee451f5a4edf3018f9f7d005

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)

comment:21 Changed 21 months ago by git

  • Commit changed from f69c9ee4feed0222ee451f5a4edf3018f9f7d005 to a1ba52a0cb17df7bc45471f7f5491798d485823a

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

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)

comment:22 Changed 20 months ago by git

  • Commit changed from a1ba52a0cb17df7bc45471f7f5491798d485823a to ce0350398939873c5273bac486dc858e42d9b497

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

2ca24ffMerge branch 'public/manifolds/top_manif_basics' of git://trac.sagemath.org/sage into Sage 6.10.rc1
0fb39dfRefactoring the code to separate out the structural part of the manifold.
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)
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

comment:23 Changed 20 months ago by git

  • Commit changed from ce0350398939873c5273bac486dc858e42d9b497 to ed4aa5867c63789c4152c1fa6dfa89bef2159795

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

comment:24 Changed 19 months ago by git

  • Commit changed from ed4aa5867c63789c4152c1fa6dfa89bef2159795 to c01048f2c51f98798ef8f86548241d69e6578d56

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

comment:25 Changed 19 months ago by egourgoulhon

  • Milestone changed from sage-6.10 to sage-7.0

comment:26 Changed 19 months ago by git

  • Commit changed from c01048f2c51f98798ef8f86548241d69e6578d56 to 8e17d54b31901cf64a63dd6284868e2c625da81d

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

af727a8Merge branch 'public/manifolds/top_manif_basics' of trac.sagemath.org:sage into public/manifolds/top_manif_basics
1113442Some final reviewer changes and tweaks.
4ed37ceSome documentation rewording and other small tweaks.
cf5e98bUse standard copyright template.
0c9d7faMerge branch 'public/manifolds/top_manif_basics' into Sage 7.1.beta0.
00d265cSet verbose=False as the default in CoordChange.set_inverse; change "coord" to "coordinates" in names of ManifoldPoint methods.
8e17d54Merge into the latest version of #18529; improve treatment of composite functions in ExpressionNice

comment:27 Changed 16 months ago by git

  • Commit changed from 8e17d54b31901cf64a63dd6284868e2c625da81d to 3b92a85394e8fc6f7572c58d0a6ce8b1bebacce2

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

c767696Merge branch 'public/manifolds/top_manif_scalar_fields' of trac.sagemath.org:sage into public/manifolds/top_manif_scalar_fields
3b92a85Some formatting and small reviewer changes.

comment:28 follow-up: Changed 16 months 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: Changed 16 months ago by egourgoulhon

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 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.

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 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: Changed 16 months ago by tscrim

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 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 16 months ago by egourgoulhon

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 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: Changed 16 months 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 16 months ago by egourgoulhon

Replying to 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)?

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 16 months ago by tscrim

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

comment:35 Changed 16 months ago by tscrim

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

comment:36 Changed 16 months ago by git

  • Commit changed from 3b92a85394e8fc6f7572c58d0a6ce8b1bebacce2 to 57b7b7b90110d1e4cc6d7e612f43cab49efc80c8

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

e44f7d9Merge branch 'develop' into public/manifolds/top_manif_scalar_fields
0fd44d6Some cleanup and creating a parent for coordinate functions.
33abe9eMarking known bugs in utilities.py.
57b7b7bImplementing global_options for display formatting.

comment:37 follow-up: Changed 16 months 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 16 months ago by tscrim

  • Reviewers set to Travis Scrimshaw

comment:39 Changed 16 months ago by git

  • Commit changed from 57b7b7b90110d1e4cc6d7e612f43cab49efc80c8 to f11df50d2cd87172375db7d3c88e420ebb09d721

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

f11df50Set CoordFunctionSymbRing in the category of commutative algebras over SR; minor documentation changes

comment:40 in reply to: ↑ 37 Changed 16 months ago by egourgoulhon

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 16 months ago by egourgoulhon

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

comment:42 follow-up: Changed 16 months 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 16 months ago by git

  • Commit changed from f11df50d2cd87172375db7d3c88e420ebb09d721 to f3f5470059519938a13905f275341d27bb5eb298

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

f3f5470Fixing a (essentially trivial) doctest failure.

comment:44 Changed 16 months ago by git

  • Commit changed from f3f5470059519938a13905f275341d27bb5eb298 to 9ec7d3e4d636676e7091f6e7032f274853677418

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

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

comment:45 in reply to: ↑ 42 Changed 16 months ago by egourgoulhon

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: Changed 16 months ago by tscrim

  • Status changed from needs_review to positive_review

Great. All is good with me. Positive review.

comment:47 in reply to: ↑ 46 Changed 16 months ago by egourgoulhon

Replying to tscrim:

Great. All is good with me. Positive review.

Thanks !

comment:48 Changed 15 months ago by vbraun

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