Opened 18 months ago
Closed 17 months ago
#31881 closed enhancement (fixed)
RealSet: Extend constructors so that they can build manifold objects
Reported by:  Matthias Köppe  Owned by:  

Priority:  major  Milestone:  sage9.4 
Component:  manifolds  Keywords:  
Cc:  Eric Gourgoulhon, Travis Scrimshaw, Michael Jung  Merged in:  
Authors:  Matthias Koeppe  Reviewers:  Michael Jung, Eric Gourgoulhon 
Report Upstream:  N/A  Work issues:  
Branch:  f85db25 (Commits, GitHub, GitLab)  Commit:  f85db25a9c30bcd91470fda5358b3d953e470233 
Dependencies:  #31688, #32089, #30473  Stopgaps: 
Description (last modified by )
From #30832:
We extend the RealSet
constructors (default constructor and RealSet.open
etc.) so that they can optionally build manifold objects; for example if structure
or name
is passed. This is done via chart pullbacks (#31688).
We deprecate the global bindings RealLine
and OpenInterval
(which will remain available through the manifolds
catalog).
This will help make the functionality more discoverable. There are just too many global constructors: Reals
, RealLine
, RealSet
, RealInterval
, OpenInterval
.
Change History (63)
comment:1 Changed 18 months ago by
Description:  modified (diff) 

comment:2 followup: 3 Changed 18 months ago by
comment:3 Changed 18 months ago by
Replying to egourgoulhon:
Replying to mkoeppe:
Overall, the
UniqueRepresentation
behavior ofRealLine
is inconsistent with the behavior ofManifold
:sage: RealLine(name='A') is RealLine(name='A') True sage: Manifold(1, name='A') is Manifold(1, name='A') FalseThis is intended because
RealLine
fully specifies the object [...]. HenceRealLine
hasUniqueRepresentation
behavior, whileManifold
has not.
I agree with this mathematical point, but on the implementation side you have additional mutable structure, namely the family of declared subsets, on every manifold. This makes the question of identity trickier.
comment:4 Changed 17 months ago by
Dependencies:  → #31688 

comment:5 Changed 17 months ago by
Branch:  → u/mkoeppe/realset__extend_constructors_so_that_they_can_build_manifold_objects 

comment:6 Changed 17 months ago by
Commit:  → 9db24e28e78c8fc10ab92482b601fda29d59bb50 

Branch pushed to git repo; I updated commit sha1. New commits:
9db24e2  RealSet: Handle keywords arguments structure, name

comment:7 Changed 17 months ago by
Dependencies:  #31688 → #31688, #32089 

comment:8 Changed 17 months ago by
Authors:  → Matthias Koeppe 

Dependencies:  #31688, #32089 → #31688, #32089, #30473 
comment:9 Changed 17 months ago by
Commit:  9db24e28e78c8fc10ab92482b601fda29d59bb50 → 6f3a9cc07299f15e7051a72274f036b0a0c95e89 

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
2a23cb5  Unicode symbol 2202 (partial) for the text display of coordinate frames

5d096f1  fstring for unicode_symbol in TensorProductFunctor and SignedTensorProductFunctor

76c2fd5  Use Unicode symbol for the Riemann sphere example

5167e6c  Use Unicode symbol for default text display of RealLine

332410b  Use unicode_otimes in TensorProductFunctor and SignedTensorProductFunctor

f5d15d2  Merge branch 'public/manifolds/unicode_art' of git://trac.sagemath.org/sage into Sage 9.4.beta4.

d87d09b  #30473: fix doctest error in DiffMap.pullback

80321fe  Merge #30473

fe56446  RealSet: Update doctest output for manifold unicode changes

6f3a9cc  RealSet: Update new doctest output for manifold unicode changes

comment:10 Changed 17 months ago by
Commit:  6f3a9cc07299f15e7051a72274f036b0a0c95e89 → 5a31e05b5f09b394edfc0b4910c23780a2c4d5a0 

comment:11 Changed 17 months ago by
Description:  modified (diff) 

comment:12 Changed 17 months ago by
Commit:  5a31e05b5f09b394edfc0b4910c23780a2c4d5a0 → 766b1412f04c50d64e0b1763a3edf38fc02ddce9 

comment:13 Changed 17 months ago by
Status:  new → needs_review 

comment:14 Changed 17 months ago by
Description:  modified (diff) 

comment:15 Changed 17 months ago by
This looks nice. Makes the access of these objects more natural. Good work!
Two things:
 For some reason, there is no documentation of the class
RealSet
. That's a pity. It would be nice to add at least anINPUT
block and explain the optional keyword(s). We should also shift the preexisting examples in__init__
to theRealSet
class documentation, where they can be seen from the official documentation.  Similarly it would be good to extend the
INPUT
block for each method with those new optional keywords accordingly (perhaps it is enough to refer toRealSet
, once this is documented). A short example per method would be preferable, too.
comment:16 Changed 17 months ago by
Commit:  766b1412f04c50d64e0b1763a3edf38fc02ddce9 → d6055c467f61fbb8fff18a1d929278d70c261682 

comment:17 Changed 17 months ago by
Commit:  d6055c467f61fbb8fff18a1d929278d70c261682 → ffe18b019a7cf47f0b2425d089f022a64c81d817 

Branch pushed to git repo; I updated commit sha1. New commits:
f2ae50e  #30473: fix doctests outside sage/manifolds and sage/tensor/modules

55240bb  Merge #30473

82f12e2  Merge #32013

0f60232  ConditionSet: Do not sort the conditions, just use _stable_uniq

69d045a  ConditionSet: In doctests, do not rename ZZ^2 etc.

ffe18b0  Merge #32089

comment:18 Changed 17 months ago by
Status:  needs_review → needs_work 

Thanks for taking a look, I'll work on the documentation.
comment:19 Changed 17 months ago by
Commit:  ffe18b019a7cf47f0b2425d089f022a64c81d817 → 4081825a28bcc30a0c4538f4f66bad59f93f2e96 

Branch pushed to git repo; I updated commit sha1. New commits:
c90b6fd  RealSet._an_element_: Define this instead of an_element, raise EmptySetError instead of ValueError, to adhere to the _an_element_ protocol

4081825  RealSet: Move docstring from __init__ to class, add TestSuite runs for more instances, including empty set

comment:20 Changed 17 months ago by
Commit:  4081825a28bcc30a0c4538f4f66bad59f93f2e96 → b7a0c77de29dd44ad80be1c47d595635e85dfa2d 

Branch pushed to git repo; I updated commit sha1. New commits:
b7a0c77  RealSet: Expand docstring

comment:21 Changed 17 months ago by
Status:  needs_work → needs_review 

comment:22 Changed 17 months ago by
Commit:  b7a0c77de29dd44ad80be1c47d595635e85dfa2d → c107b321c9ca6125f6c8d63374a2db187a535f54 

Branch pushed to git repo; I updated commit sha1. New commits:
c107b32  ManifoldSubsetPullback: Fix pullback of scalar sets by 1dimensional chart

comment:23 followup: 24 Changed 17 months ago by
That is much better, thank you. Minor things:
 Perhaps it is good to clarify in the docstring that
RealSet
now acts as a constructor.  I am not sure whether I like that
RealSet(0, 1, name='A')
returns a manifold object. This is too subtle imho. Perhaps an error message is more appropriate, explaining that e.g.structure=differentiable
is necessary to pass in those cases?
comment:24 followups: 25 26 Changed 17 months ago by
Replying to ghmjungmath:
I am not sure whether I like that
RealSet(0, 1, name='A')
returns a manifold object. This is too subtle imho. Perhaps an error message is more appropriate, explaining that e.g.structure=differentiable
is necessary to pass in those cases?
I agree that triggering it with name
and latex_name
may be too subtle.
OTOH, I think if ambient
is passed, structure
should be implied.
How about the parameters coordinate
, names
, start_index
?
comment:25 followup: 27 Changed 17 months ago by
Replying to mkoeppe:
Replying to ghmjungmath:
I am not sure whether I like that
RealSet(0, 1, name='A')
returns a manifold object. This is too subtle imho. Perhaps an error message is more appropriate, explaining that e.g.structure=differentiable
is necessary to pass in those cases?I agree that triggering it with
name
andlatex_name
may be too subtle.OTOH, I think if
ambient
is passed,structure
should be implied.How about the parameters
coordinate
,names
,start_index
?
In connection to this, having something like R.<t> = RealLine()
was very handy, especially when dealing with curves on manifolds. How do you plan to replace it with the RealSet
syntax?
comment:26 followup: 28 Changed 17 months ago by
Replying to mkoeppe:
Replying to ghmjungmath:
I am not sure whether I like that
RealSet(0, 1, name='A')
returns a manifold object. This is too subtle imho. Perhaps an error message is more appropriate, explaining that e.g.structure=differentiable
is necessary to pass in those cases?I agree that triggering it with
name
andlatex_name
may be too subtle.OTOH, I think if
ambient
is passed,structure
should be implied.
I am torn on that one but I tend to agree.
How about the parameters
coordinate
,names
,start_index
?
coordinate
should be clear but the rest seems similarly ambiguous to me.
comment:27 Changed 17 months ago by
Replying to egourgoulhon:
having something like
R.<t> = RealLine()
was very handy, especially when dealing with curves on manifolds. How do you plan to replace it with theRealSet
syntax?
Strangely, RealSet
does not have a shortcut constructor for the whole real line  one has to write RealSet(oo, oo)
.
We can add one here on this ticket. RealSet.real_line()
, RealSet.universe()
, ...? Any preference?
comment:28 Changed 17 months ago by
Replying to ghmjungmath:
How about the parameters
coordinate
,names
,start_index
?
coordinate
should be clear but the rest seems similarly ambiguous to me.
Well, names
is for the generator syntax, so it would be rather convenient for it to trigger manifold construction
comment:29 followup: 30 Changed 17 months ago by
Commit:  c107b321c9ca6125f6c8d63374a2db187a535f54 → 4f57a29edd4ad4b77690ddd94f18cf9d8d7c6417 

comment:30 followup: 31 Changed 17 months ago by
Replying to git:
Branch pushed to git repo; I updated commit sha1. New commits:
b9b7983 RealSet: Minor edits to docstring
4f57a29 RealSet: Restrict keywords that trigger manifold construction
The explanation in the INPUT
block should now be modified accordingly, mentioning that (only) ambient
, coordinate
, names
trigger manifold constructions without imposing structure=differentiable
.
Perhaps an example where the new error message is invoked might be a good idea, too.
Replying to mkoeppe:
We can add one here on this ticket.
RealSet.real_line()
,RealSet.universe()
, ...? Any preference?
I like both. But to keep some similarity with the previous constructor name RealLine
, I'd say RealSet.real_line()
should definitely be on the list.
comment:31 Changed 17 months ago by
Replying to ghmjungmath:
Replying to mkoeppe:
We can add one here on this ticket.
RealSet.real_line()
,RealSet.universe()
, ...? Any preference?I like both. But to keep some similarity with the previous constructor name
RealLine
, I'd sayRealSet.real_line()
should definitely be on the list.
+1
comment:32 Changed 17 months ago by
Commit:  4f57a29edd4ad4b77690ddd94f18cf9d8d7c6417 → 199f52e6ca29de671470145d4709384b5c6abaa8 

comment:33 Changed 17 months ago by
Two comments:
 ``structure``  (default: ``None``) if ``None``, construct the real set as an instance of :class:`RealSet`; if ``"differentiable"``, construct it as a subset of  the differentiable manifold. + the differentiable manifold `\RR`.
An example a la
sage: R.<ξ> = RealSet.real_line(); R ...
would be nice, too.
comment:35 Changed 17 months ago by
Commit:  199f52e6ca29de671470145d4709384b5c6abaa8 → 4301940822a9c9804a35c86954a30af8f498fa91 

Branch pushed to git repo; I updated commit sha1. New commits:
4301940  RealSet: Minor docstring edits

comment:36 Changed 17 months ago by
Commit:  4301940822a9c9804a35c86954a30af8f498fa91 → c91e2ca8dfb2bfbbe97977ecc8af71ca4c67ea7e 

Branch pushed to git repo; I updated commit sha1. New commits:
c91e2ca  RealSet constructors: Add reference for **kwds

comment:39 Changed 17 months ago by
It looks like we have to play patchbot ourselves now. I'll run the branch tomorrow (CET) on my computer.
comment:40 Changed 17 months ago by
Docbuild seems not to be satisfied:
[dochtml] [manifolds] /home/michael/GitProjects/sagedevel/local/lib/python3.8/sitepackages/matplotlib/sphinxext/plot_directive.py:472: DeprecationWarning: [dochtml] [manifolds] Importing RealLine from here is deprecated. If you need to use it, please import it directly from sage.manifolds.differentiable.examples.real_line [dochtml] [manifolds] See https://trac.sagemath.org/31881 for details. [dochtml] [manifolds] exec(code, ns) [dochtml] [manifolds] <unknown>:954: DeprecationWarning: invalid escape sequence \
comment:41 Changed 17 months ago by
Among other things, the doctest returns
File "src/sage/interfaces/sympy_wrapper.py", line 71, in sage.interfaces.sympy_wrapper.SageSet._sage_ Failed example: sF = F._sympy_(); sF Expected: SageSet({1, 2}) Got: Set(1, 2)
Is that related to #32089?
comment:42 Changed 17 months ago by
Commit:  c91e2ca8dfb2bfbbe97977ecc8af71ca4c67ea7e → 9b71c9c26c010142842d4aed3f113954c32765fa 

comment:43 followup: 46 Changed 17 months ago by
comment:44 Changed 17 months ago by
Patchbot is back! Most of its issues seem unrelated to this ticket, tho.
What about the docbuild warning in comment:40?
comment:45 Changed 17 months ago by
Commit:  9b71c9c26c010142842d4aed3f113954c32765fa → 39e25866dd63641a67c110aa0b2fcd7bc0125c1e 

Branch pushed to git repo; I updated commit sha1. New commits:
2682469  src/sage/interfaces/sympy_wrapper.py: Use Family, not Set, in doctests to make sure that the SageSet wrapper is actually used

753babb  Set_object_enumerated._sympy_: Translate empty sets to EmptySet

141ecde  Merge #32015

39e2586  Merge #32089

comment:46 Changed 17 months ago by
comment:47 Changed 17 months ago by
Commit:  39e25866dd63641a67c110aa0b2fcd7bc0125c1e → cc3aa94db91394f183100d0bea8c6babaeefe532 

Branch pushed to git repo; I updated commit sha1. New commits:
cc3aa94  RealSet: Fix up docstring, add table

comment:48 Changed 17 months ago by
As soon as the warning in comment:40 is removed, I give a positive review.
It looks like RealLine
is still imported via the global namespace in one of the examples?
comment:49 Changed 17 months ago by
Commit:  cc3aa94db91394f183100d0bea8c6babaeefe532 → cb307e1ad534c9221aa90449170a22c9a714d5da 

Branch pushed to git repo; I updated commit sha1. New commits:
cb307e1  src/sage/manifolds/differentiable/curve.py: Update imports of RealLine in PLOT directives

comment:50 Changed 17 months ago by
Status:  needs_review → needs_work 

 Returns the symmetric difference of ``self`` and ``other``. + Return the symmetric difference of ``self`` and ``other``.
comment:51 Changed 17 months ago by
Status:  needs_work → needs_review 

That is just coming along for the ride with moving things, and even if not, it is very minor that I would not hold up a positive review for it.
comment:52 Changed 17 months ago by
It's good to maintain some consistency. Besides, the patchbot doesn't like it. So it must be of some importance.
I am just afraid: if we don't fix it here, we probably will never...
comment:53 Changed 17 months ago by
You can push a review commit or do a general cleanup ticket later on. It is there in the patchbot as advice to the reviewer. You have some discretion about these things.
comment:54 Changed 17 months ago by
Status:  needs_review → positive_review 

As proposed by Travis, here is a cleanup followup ticket: #32192.
LGTM.
comment:55 Changed 17 months ago by
Reviewers:  → Michael Jung, Eric Gourgoulhon 

comment:58 Changed 17 months ago by
Commit:  cb307e1ad534c9221aa90449170a22c9a714d5da → 8093a4d772b6ef51164cfe95a68eb5d4e8ea5a23 

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
80f6195  Chart, RealChart: In class docstring, order arguments as they appear in __classcall__/__init__

a39e6fc  DiffChart, RealDiffChart: In class docstring, order arguments as they appear in __classcall__/__init__; add description of argument coord_restrictions

cdf20b0  TopologicalManifold.chart: Add description of argument coord_restrictions

741fd2e  TopologicalManifold.chart: Add an example of using coord_restrictions

bf62543  Merge branch 't/32089/conditionset__conditionset_callable_symbolic_expression' into t/32009/eliminate_direct_use_of_the_chart__domain_attribute

141ccb5  Merge #32009

c89c697  Merge #32102

451f5cf  Sets.ParentMethods: Update doctest

f135a05  Merge #32015

8093a4d  Merge #32089

comment:59 Changed 17 months ago by
Status:  needs_work → positive_review 

Works for me after merging the lastest versions of the dependencies
comment:60 Changed 17 months ago by
Status:  positive_review → needs_work 

Still borked. Make sure you merge in the next beta before changing the state again.
comment:61 Changed 17 months ago by
Commit:  8093a4d772b6ef51164cfe95a68eb5d4e8ea5a23 → f85db25a9c30bcd91470fda5358b3d953e470233 

comment:63 Changed 17 months ago by
Branch:  u/mkoeppe/realset__extend_constructors_so_that_they_can_build_manifold_objects → f85db25a9c30bcd91470fda5358b3d953e470233 

Resolution:  → fixed 
Status:  positive_review → closed 
Replying to mkoeppe:
This is intended because
RealLine
fully specifies the object (the set of real numbers endowed with the canonical manifold structure), whileManifold(1, name='A')
does not (since there are two distinct manifolds of dimension 1: the real line and the circle). HenceRealLine
hasUniqueRepresentation
behavior, whileManifold
has not.Agreed.
Indeed!