#31881 closed enhancement (fixed)

RealSet: Extend constructors so that they can build manifold objects

Reported by: Matthias Köppe Owned by:
Priority: major Milestone: sage-9.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:

Status badges

Description (last modified by Matthias Köppe)

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 Matthias Köppe

Description: modified (diff)

comment:2 in reply to:  description ; Changed 18 months ago by Eric Gourgoulhon

Replying to mkoeppe:

Overall, the UniqueRepresentation behavior of RealLine is inconsistent with the behavior of Manifold:

sage: RealLine(name='A') is RealLine(name='A')
True
sage: Manifold(1, name='A') is Manifold(1, name='A')
False

This is intended because RealLine fully specifies the object (the set of real numbers endowed with the canonical manifold structure), while Manifold(1, name='A') does not (since there are two distinct manifolds of dimension 1: the real line and the circle). Hence RealLine has UniqueRepresentation behavior, while Manifold has not.

We also deprecate the global binding OpenInterval (but it will remain available through the manifolds catalog).

Agreed.

This will help make the functionality more discoverable. There are just too many global constructors: Reals, RealLine, RealSet, RealInterval, OpenInterval.

Indeed!

comment:3 in reply to:  2 Changed 18 months ago by Matthias Köppe

Replying to egourgoulhon:

Replying to mkoeppe:

Overall, the UniqueRepresentation behavior of RealLine is inconsistent with the behavior of Manifold:

sage: RealLine(name='A') is RealLine(name='A')
True
sage: Manifold(1, name='A') is Manifold(1, name='A')
False

This is intended because RealLine fully specifies the object [...]. Hence RealLine has UniqueRepresentation behavior, while Manifold 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 Matthias Köppe

Dependencies: #31688

comment:5 Changed 17 months ago by Matthias Köppe

Branch: u/mkoeppe/realset__extend_constructors_so_that_they_can_build_manifold_objects

comment:6 Changed 17 months ago by git

Commit: 9db24e28e78c8fc10ab92482b601fda29d59bb50

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

9db24e2RealSet: Handle keywords arguments structure, name

comment:7 Changed 17 months ago by Matthias Köppe

Dependencies: #31688#31688, #32089

comment:8 Changed 17 months ago by Matthias Köppe

Authors: Matthias Koeppe
Dependencies: #31688, #32089#31688, #32089, #30473

comment:9 Changed 17 months ago by git

Commit: 9db24e28e78c8fc10ab92482b601fda29d59bb506f3a9cc07299f15e7051a72274f036b0a0c95e89

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

2a23cb5Unicode symbol 2202 (partial) for the text display of coordinate frames
5d096f1f-string for unicode_symbol in TensorProductFunctor and SignedTensorProductFunctor
76c2fd5Use Unicode symbol for the Riemann sphere example
5167e6cUse Unicode symbol for default text display of RealLine
332410bUse unicode_otimes in TensorProductFunctor and SignedTensorProductFunctor
f5d15d2Merge branch 'public/manifolds/unicode_art' of git://trac.sagemath.org/sage into Sage 9.4.beta4.
d87d09b#30473: fix doctest error in DiffMap.pullback
80321feMerge #30473
fe56446RealSet: Update doctest output for manifold unicode changes
6f3a9ccRealSet: Update new doctest output for manifold unicode changes

comment:10 Changed 17 months ago by git

Commit: 6f3a9cc07299f15e7051a72274f036b0a0c95e895a31e05b5f09b394edfc0b4910c23780a2c4d5a0

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

cd83629OpenInterval, RealLineDeprecate global bindings, replace all uses in doctests from manifolds catalog
ac561aeChart._restrict_set: Update doctest output
5a31e05Merge #32089

comment:11 Changed 17 months ago by Matthias Köppe

Description: modified (diff)

comment:12 Changed 17 months ago by git

Commit: 5a31e05b5f09b394edfc0b4910c23780a2c4d5a0766b1412f04c50d64e0b1763a3edf38fc02ddce9

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

5571025RealSet: Use ambient.manifold().canonical_chart(), add nested example
766b141RealSet: Add another example

comment:13 Changed 17 months ago by Matthias Köppe

Status: newneeds_review

comment:14 Changed 17 months ago by Matthias Köppe

Description: modified (diff)

comment:15 Changed 17 months ago by Michael Jung

This looks nice. Makes the access of these objects more natural. Good work!

Two things:

  1. For some reason, there is no documentation of the class RealSet. That's a pity. It would be nice to add at least an INPUT block and explain the optional keyword(s). We should also shift the preexisting examples in __init__ to the RealSet class documentation, where they can be seen from the official documentation.
  2. 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 to RealSet, once this is documented). A short example per method would be preferable, too.
Last edited 17 months ago by Michael Jung (previous) (diff)

comment:16 Changed 17 months ago by git

Commit: 766b1412f04c50d64e0b1763a3edf38fc02ddce9d6055c467f61fbb8fff18a1d929278d70c261682

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

4558e26Polyhedron_base.incidence_matrix: Remove accidental change to doctest
b537f1bMerge #31688
63c2cfeConvexSet_base._test_convex_set: Do not test _test_as_set_object here
4844cd6Merge #32013
d6055c4Merge #32089

comment:17 Changed 17 months ago by git

Commit: d6055c467f61fbb8fff18a1d929278d70c261682ffe18b019a7cf47f0b2425d089f022a64c81d817

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

f2ae50e#30473: fix doctests outside sage/manifolds and sage/tensor/modules
55240bbMerge #30473
82f12e2Merge #32013
0f60232ConditionSet: Do not sort the conditions, just use _stable_uniq
69d045aConditionSet: In doctests, do not rename ZZ^2 etc.
ffe18b0Merge #32089

comment:18 Changed 17 months ago by Matthias Köppe

Status: needs_reviewneeds_work

Thanks for taking a look, I'll work on the documentation.

comment:19 Changed 17 months ago by git

Commit: ffe18b019a7cf47f0b2425d089f022a64c81d8174081825a28bcc30a0c4538f4f66bad59f93f2e96

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

c90b6fdRealSet._an_element_: Define this instead of an_element, raise EmptySetError instead of ValueError, to adhere to the _an_element_ protocol
4081825RealSet: Move docstring from __init__ to class, add TestSuite runs for more instances, including empty set

comment:20 Changed 17 months ago by git

Commit: 4081825a28bcc30a0c4538f4f66bad59f93f2e96b7a0c77de29dd44ad80be1c47d595635e85dfa2d

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

b7a0c77RealSet: Expand docstring

comment:21 Changed 17 months ago by Matthias Köppe

Status: needs_workneeds_review

comment:22 Changed 17 months ago by git

Commit: b7a0c77de29dd44ad80be1c47d595635e85dfa2dc107b321c9ca6125f6c8d63374a2db187a535f54

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

c107b32ManifoldSubsetPullback: Fix pullback of scalar sets by 1-dimensional chart

comment:23 Changed 17 months ago by Michael Jung

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 ; Changed 17 months ago by Matthias Köppe

Replying to gh-mjungmath:

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?

Last edited 17 months ago by Matthias Köppe (previous) (diff)

comment:25 in reply to:  24 ; Changed 17 months ago by Eric Gourgoulhon

Replying to mkoeppe:

Replying to gh-mjungmath:

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?

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 ; Changed 17 months ago by Michael Jung

Replying to mkoeppe:

Replying to gh-mjungmath:

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.

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 17 months ago by Matthias Köppe

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 the RealSet 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?

Last edited 17 months ago by Matthias Köppe (previous) (diff)

comment:28 in reply to:  26 Changed 17 months ago by Matthias Köppe

Replying to gh-mjungmath:

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 Changed 17 months ago by git

Commit: c107b321c9ca6125f6c8d63374a2db187a535f544f57a29edd4ad4b77690ddd94f18cf9d8d7c6417

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

b9b7983RealSet: Minor edits to docstring
4f57a29RealSet: Restrict keywords that trigger manifold construction

comment:30 in reply to:  29 ; Changed 17 months ago by Michael Jung

Replying to git:

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

b9b7983RealSet: Minor edits to docstring
4f57a29RealSet: 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.

Last edited 17 months ago by Michael Jung (previous) (diff)

comment:31 in reply to:  30 Changed 17 months ago by Eric Gourgoulhon

Replying to gh-mjungmath:

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.

+1

comment:32 Changed 17 months ago by git

Commit: 4f57a29edd4ad4b77690ddd94f18cf9d8d7c6417199f52e6ca29de671470145d4709384b5c6abaa8

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

47b1e40RealSet.real_line: New
199f52eRealSet: Update documentation of manifold keywords; make passing None the same as not passing the keyword

comment:33 Changed 17 months ago by Michael Jung

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 17 months ago by Michael Jung

Other than that, LGTM. Eric? Any comments from your side?

comment:35 Changed 17 months ago by git

Commit: 199f52e6ca29de671470145d4709384b5c6abaa84301940822a9c9804a35c86954a30af8f498fa91

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

4301940RealSet: Minor docstring edits

comment:36 Changed 17 months ago by git

Commit: 4301940822a9c9804a35c86954a30af8f498fa91c91e2ca8dfb2bfbbe97977ecc8af71ca4c67ea7e

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

c91e2caRealSet constructors: Add reference for **kwds

comment:37 Changed 17 months ago by Michael Jung

Ready for review?

comment:38 Changed 17 months ago by Matthias Köppe

Yes

comment:39 Changed 17 months ago by Michael Jung

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 Michael Jung

Docbuild seems not to be satisfied:

[dochtml] [manifolds] /home/michael/GitProjects/sage-devel/local/lib/python3.8/site-packages/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 \|
Last edited 17 months ago by Michael Jung (previous) (diff)

comment:41 Changed 17 months ago by Michael Jung

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 git

Commit: c91e2ca8dfb2bfbbe97977ecc8af71ca4c67ea7e9b71c9c26c010142842d4aed3f113954c32765fa

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

daeb91esrc/sage/sets/set.py: Fix docstring markup
2cf2199Merge #32015
1eb270asrc/sage/docs/conf.py: Add more \ensuremath to \DeclareUnicodeCharacter
9b71c9cMerge #32089

comment:43 Changed 17 months ago by Matthias Köppe

Yes, just fixed this in #32015. Merged now via updated #32089. Thanks for testing!

comment:44 Changed 17 months ago by Michael Jung

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 git

Commit: 9b71c9c26c010142842d4aed3f113954c32765fa39e25866dd63641a67c110aa0b2fcd7bc0125c1e

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

2682469src/sage/interfaces/sympy_wrapper.py: Use Family, not Set, in doctests to make sure that the SageSet wrapper is actually used
753babbSet_object_enumerated._sympy_: Translate empty sets to EmptySet
141ecdeMerge #32015
39e2586Merge #32089

comment:46 in reply to:  43 Changed 17 months ago by Matthias Köppe

Replying to mkoeppe:

Yes, just fixed this in #32015. Merged now via updated #32089. Thanks for testing!

actually, now it's merged

comment:47 Changed 17 months ago by git

Commit: 39e25866dd63641a67c110aa0b2fcd7bc0125c1ecc3aa94db91394f183100d0bea8c6babaeefe532

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

cc3aa94RealSet: Fix up docstring, add table

comment:48 Changed 17 months ago by Michael Jung

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 git

Commit: cc3aa94db91394f183100d0bea8c6babaeefe532cb307e1ad534c9221aa90449170a22c9a714d5da

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

cb307e1src/sage/manifolds/differentiable/curve.py: Update imports of RealLine in PLOT directives

comment:50 Changed 17 months ago by Michael Jung

Status: needs_reviewneeds_work
-        Returns the symmetric difference of ``self`` and ``other``.
+        Return the symmetric difference of ``self`` and ``other``.

See https://wiki.sagemath.org/plugins#blocks.

comment:51 Changed 17 months ago by Travis Scrimshaw

Status: needs_workneeds_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 Michael Jung

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 Travis Scrimshaw

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 Michael Jung

Status: needs_reviewpositive_review

As proposed by Travis, here is a clean-up follow-up ticket: #32192.

LGTM.

comment:55 Changed 17 months ago by Michael Jung

Reviewers: Michael Jung, Eric Gourgoulhon

comment:56 Changed 17 months ago by Matthias Köppe

Thanks!

comment:57 Changed 17 months ago by Volker Braun

Status: positive_reviewneeds_work

PDF docs don't build

comment:58 Changed 17 months ago by git

Commit: cb307e1ad534c9221aa90449170a22c9a714d5da8093a4d772b6ef51164cfe95a68eb5d4e8ea5a23

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

80f6195Chart, RealChart: In class docstring, order arguments as they appear in __classcall__/__init__
a39e6fcDiffChart, RealDiffChart: In class docstring, order arguments as they appear in __classcall__/__init__; add description of argument coord_restrictions
cdf20b0TopologicalManifold.chart: Add description of argument coord_restrictions
741fd2eTopologicalManifold.chart: Add an example of using coord_restrictions
bf62543Merge branch 't/32089/conditionset__conditionset_callable_symbolic_expression' into t/32009/eliminate_direct_use_of_the_chart__domain_attribute
141ccb5Merge #32009
c89c697Merge #32102
451f5cfSets.ParentMethods: Update doctest
f135a05Merge #32015
8093a4dMerge #32089

comment:59 Changed 17 months ago by Matthias Köppe

Status: needs_workpositive_review

Works for me after merging the lastest versions of the dependencies

comment:60 Changed 17 months ago by Volker Braun

Status: positive_reviewneeds_work

Still borked. Make sure you merge in the next beta before changing the state again.

comment:61 Changed 17 months ago by git

Commit: 8093a4d772b6ef51164cfe95a68eb5d4e8ea5a23f85db25a9c30bcd91470fda5358b3d953e470233

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

3ce89e8Merge tag '9.4.beta6' into t/31881/realset__extend_constructors_so_that_they_can_build_manifold_objects
f85db25src/sage/docs/conf.py: Add more \DeclareUnicodeCharacter

comment:62 Changed 17 months ago by Matthias Köppe

Status: needs_workpositive_review

OK, found it, fixed it

comment:63 Changed 17 months ago by Volker Braun

Branch: u/mkoeppe/realset__extend_constructors_so_that_they_can_build_manifold_objectsf85db25a9c30bcd91470fda5358b3d953e470233
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.