Opened 12 months ago
Closed 10 months ago
#31881 closed enhancement (fixed)
RealSet: Extend constructors so that they can build manifold objects
Reported by:  mkoeppe  Owned by:  

Priority:  major  Milestone:  sage9.4 
Component:  manifolds  Keywords:  
Cc:  egourgoulhon, tscrim, ghmjungmath  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 11 months ago by
 Description modified (diff)
comment:2 in reply to: ↑ description ; followup: ↓ 3 Changed 11 months ago by
comment:3 in reply to: ↑ 2 Changed 11 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 11 months ago by
 Dependencies set to #31688
comment:5 Changed 11 months ago by
 Branch set to u/mkoeppe/realset__extend_constructors_so_that_they_can_build_manifold_objects
comment:6 Changed 11 months ago by
 Commit set to 9db24e28e78c8fc10ab92482b601fda29d59bb50
Branch pushed to git repo; I updated commit sha1. New commits:
9db24e2  RealSet: Handle keywords arguments structure, name

comment:7 Changed 11 months ago by
 Dependencies changed from #31688 to #31688, #32089
comment:8 Changed 11 months ago by
 Dependencies changed from #31688, #32089 to #31688, #32089, #30473
comment:9 Changed 11 months ago by
 Commit changed from 9db24e28e78c8fc10ab92482b601fda29d59bb50 to 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 11 months ago by
 Commit changed from 6f3a9cc07299f15e7051a72274f036b0a0c95e89 to 5a31e05b5f09b394edfc0b4910c23780a2c4d5a0
comment:11 Changed 11 months ago by
 Description modified (diff)
comment:12 Changed 11 months ago by
 Commit changed from 5a31e05b5f09b394edfc0b4910c23780a2c4d5a0 to 766b1412f04c50d64e0b1763a3edf38fc02ddce9
comment:13 Changed 11 months ago by
 Status changed from new to needs_review
comment:14 Changed 11 months ago by
 Description modified (diff)
comment:15 Changed 11 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 11 months ago by
 Commit changed from 766b1412f04c50d64e0b1763a3edf38fc02ddce9 to d6055c467f61fbb8fff18a1d929278d70c261682
comment:17 Changed 11 months ago by
 Commit changed from d6055c467f61fbb8fff18a1d929278d70c261682 to 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 11 months ago by
 Status changed from needs_review to needs_work
Thanks for taking a look, I'll work on the documentation.
comment:19 Changed 11 months ago by
 Commit changed from ffe18b019a7cf47f0b2425d089f022a64c81d817 to 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 11 months ago by
 Commit changed from 4081825a28bcc30a0c4538f4f66bad59f93f2e96 to b7a0c77de29dd44ad80be1c47d595635e85dfa2d
Branch pushed to git repo; I updated commit sha1. New commits:
b7a0c77  RealSet: Expand docstring

comment:21 Changed 11 months ago by
 Status changed from needs_work to needs_review
comment:22 Changed 11 months ago by
 Commit changed from b7a0c77de29dd44ad80be1c47d595635e85dfa2d to 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 11 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 in reply to: ↑ 23 ; followups: ↓ 25 ↓ 26 Changed 11 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 in reply to: ↑ 24 ; followup: ↓ 27 Changed 11 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 in reply to: ↑ 24 ; followup: ↓ 28 Changed 11 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 in reply to: ↑ 25 Changed 11 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 in reply to: ↑ 26 Changed 11 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 11 months ago by
 Commit changed from c107b321c9ca6125f6c8d63374a2db187a535f54 to 4f57a29edd4ad4b77690ddd94f18cf9d8d7c6417
comment:30 in reply to: ↑ 29 ; followup: ↓ 31 Changed 11 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 in reply to: ↑ 30 Changed 11 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 11 months ago by
 Commit changed from 4f57a29edd4ad4b77690ddd94f18cf9d8d7c6417 to 199f52e6ca29de671470145d4709384b5c6abaa8
comment:33 Changed 11 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:34 Changed 11 months ago by
Other than that, LGTM. Eric? Any comments from your side?
comment:35 Changed 11 months ago by
 Commit changed from 199f52e6ca29de671470145d4709384b5c6abaa8 to 4301940822a9c9804a35c86954a30af8f498fa91
Branch pushed to git repo; I updated commit sha1. New commits:
4301940  RealSet: Minor docstring edits

comment:36 Changed 11 months ago by
 Commit changed from 4301940822a9c9804a35c86954a30af8f498fa91 to c91e2ca8dfb2bfbbe97977ecc8af71ca4c67ea7e
Branch pushed to git repo; I updated commit sha1. New commits:
c91e2ca  RealSet constructors: Add reference for **kwds

comment:37 Changed 11 months ago by
Ready for review?
comment:38 Changed 11 months ago by
Yes
comment:39 Changed 11 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 11 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 11 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 11 months ago by
 Commit changed from c91e2ca8dfb2bfbbe97977ecc8af71ca4c67ea7e to 9b71c9c26c010142842d4aed3f113954c32765fa
comment:43 followup: ↓ 46 Changed 11 months ago by
comment:44 Changed 10 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 10 months ago by
 Commit changed from 9b71c9c26c010142842d4aed3f113954c32765fa to 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 in reply to: ↑ 43 Changed 10 months ago by
comment:47 Changed 10 months ago by
 Commit changed from 39e25866dd63641a67c110aa0b2fcd7bc0125c1e to cc3aa94db91394f183100d0bea8c6babaeefe532
Branch pushed to git repo; I updated commit sha1. New commits:
cc3aa94  RealSet: Fix up docstring, add table

comment:48 Changed 10 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 10 months ago by
 Commit changed from cc3aa94db91394f183100d0bea8c6babaeefe532 to 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 10 months ago by
 Status changed from needs_review to needs_work
 Returns the symmetric difference of ``self`` and ``other``. + Return the symmetric difference of ``self`` and ``other``.
comment:51 Changed 10 months ago by
 Status changed from needs_work to 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 10 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 10 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 10 months ago by
 Status changed from needs_review to positive_review
As proposed by Travis, here is a cleanup followup ticket: #32192.
LGTM.
comment:55 Changed 10 months ago by
 Reviewers set to Michael Jung, Eric Gourgoulhon
comment:56 Changed 10 months ago by
Thanks!
comment:57 Changed 10 months ago by
 Status changed from positive_review to needs_work
PDF docs don't build
comment:58 Changed 10 months ago by
 Commit changed from cb307e1ad534c9221aa90449170a22c9a714d5da to 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 10 months ago by
 Status changed from needs_work to positive_review
Works for me after merging the lastest versions of the dependencies
comment:60 Changed 10 months ago by
 Status changed from positive_review to needs_work
Still borked. Make sure you merge in the next beta before changing the state again.
comment:61 Changed 10 months ago by
 Commit changed from 8093a4d772b6ef51164cfe95a68eb5d4e8ea5a23 to f85db25a9c30bcd91470fda5358b3d953e470233
comment:62 Changed 10 months ago by
 Status changed from needs_work to positive_review
OK, found it, fixed it
comment:63 Changed 10 months ago by
 Branch changed from u/mkoeppe/realset__extend_constructors_so_that_they_can_build_manifold_objects to f85db25a9c30bcd91470fda5358b3d953e470233
 Resolution set to fixed
 Status changed from positive_review to 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!