Opened 5 months ago

Closed 3 months ago

#31881 closed enhancement (fixed)

RealSet: Extend constructors so that they can build manifold objects

Reported by: mkoeppe Owned by:
Priority: major Milestone: sage-9.4
Component: manifolds Keywords:
Cc: egourgoulhon, tscrim, gh-mjungmath 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 mkoeppe)

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 4 months ago by mkoeppe

  • Description modified (diff)

comment:2 in reply to: ↑ description ; follow-up: Changed 4 months ago by 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 (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 4 months ago by mkoeppe

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 3 months ago by mkoeppe

  • Dependencies set to #31688

comment:5 Changed 3 months ago by mkoeppe

  • Branch set to u/mkoeppe/realset__extend_constructors_so_that_they_can_build_manifold_objects

comment:6 Changed 3 months ago by git

  • Commit set to 9db24e28e78c8fc10ab92482b601fda29d59bb50

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

9db24e2RealSet: Handle keywords arguments structure, name

comment:7 Changed 3 months ago by mkoeppe

  • Dependencies changed from #31688 to #31688, #32089

comment:8 Changed 3 months ago by mkoeppe

  • Authors set to Matthias Koeppe
  • Dependencies changed from #31688, #32089 to #31688, #32089, #30473

comment:9 Changed 3 months ago by git

  • Commit changed from 9db24e28e78c8fc10ab92482b601fda29d59bb50 to 6f3a9cc07299f15e7051a72274f036b0a0c95e89

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

  • Commit changed from 6f3a9cc07299f15e7051a72274f036b0a0c95e89 to 5a31e05b5f09b394edfc0b4910c23780a2c4d5a0

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 3 months ago by mkoeppe

  • Description modified (diff)

comment:12 Changed 3 months ago by git

  • Commit changed from 5a31e05b5f09b394edfc0b4910c23780a2c4d5a0 to 766b1412f04c50d64e0b1763a3edf38fc02ddce9

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 3 months ago by mkoeppe

  • Status changed from new to needs_review

comment:14 Changed 3 months ago by mkoeppe

  • Description modified (diff)

comment:15 Changed 3 months ago by gh-mjungmath

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 3 months ago by gh-mjungmath (previous) (diff)

comment:16 Changed 3 months ago by git

  • Commit changed from 766b1412f04c50d64e0b1763a3edf38fc02ddce9 to d6055c467f61fbb8fff18a1d929278d70c261682

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

  • 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
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 3 months ago by mkoeppe

  • Status changed from needs_review to needs_work

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

comment:19 Changed 3 months ago by git

  • Commit changed from ffe18b019a7cf47f0b2425d089f022a64c81d817 to 4081825a28bcc30a0c4538f4f66bad59f93f2e96

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

  • Commit changed from 4081825a28bcc30a0c4538f4f66bad59f93f2e96 to b7a0c77de29dd44ad80be1c47d595635e85dfa2d

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

b7a0c77RealSet: Expand docstring

comment:21 Changed 3 months ago by mkoeppe

  • Status changed from needs_work to needs_review

comment:22 Changed 3 months ago by git

  • Commit changed from b7a0c77de29dd44ad80be1c47d595635e85dfa2d to c107b321c9ca6125f6c8d63374a2db187a535f54

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

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

comment:23 follow-up: Changed 3 months ago by gh-mjungmath

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 ; follow-ups: Changed 3 months ago by 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?

Last edited 3 months ago by mkoeppe (previous) (diff)

comment:25 in reply to: ↑ 24 ; follow-up: Changed 3 months ago by egourgoulhon

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 ; follow-up: Changed 3 months ago by gh-mjungmath

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 3 months ago by mkoeppe

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 3 months ago by mkoeppe (previous) (diff)

comment:28 in reply to: ↑ 26 Changed 3 months ago by mkoeppe

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

  • Commit changed from c107b321c9ca6125f6c8d63374a2db187a535f54 to 4f57a29edd4ad4b77690ddd94f18cf9d8d7c6417

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 ; follow-up: Changed 3 months ago by gh-mjungmath

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 3 months ago by gh-mjungmath (previous) (diff)

comment:31 in reply to: ↑ 30 Changed 3 months ago by egourgoulhon

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

  • Commit changed from 4f57a29edd4ad4b77690ddd94f18cf9d8d7c6417 to 199f52e6ca29de671470145d4709384b5c6abaa8

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 3 months ago by gh-mjungmath

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 3 months ago by gh-mjungmath

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

comment:35 Changed 3 months ago by git

  • Commit changed from 199f52e6ca29de671470145d4709384b5c6abaa8 to 4301940822a9c9804a35c86954a30af8f498fa91

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

4301940RealSet: Minor docstring edits

comment:36 Changed 3 months ago by git

  • Commit changed from 4301940822a9c9804a35c86954a30af8f498fa91 to c91e2ca8dfb2bfbbe97977ecc8af71ca4c67ea7e

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

c91e2caRealSet constructors: Add reference for **kwds

comment:37 Changed 3 months ago by gh-mjungmath

Ready for review?

comment:38 Changed 3 months ago by mkoeppe

Yes

comment:39 Changed 3 months ago by gh-mjungmath

It looks like we have to play patchbot ourselves now. I'll run the branch tomorrow (CET) on my computer.

comment:40 Changed 3 months ago by gh-mjungmath

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 3 months ago by gh-mjungmath (previous) (diff)

comment:41 Changed 3 months ago by gh-mjungmath

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

  • Commit changed from c91e2ca8dfb2bfbbe97977ecc8af71ca4c67ea7e to 9b71c9c26c010142842d4aed3f113954c32765fa

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 follow-up: Changed 3 months ago by mkoeppe

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

comment:44 Changed 3 months ago by gh-mjungmath

Patchbot is back! Most of its issues seem unrelated to this ticket, tho.

What about the docbuild warning in comment:40?

comment:45 Changed 3 months ago by git

  • Commit changed from 9b71c9c26c010142842d4aed3f113954c32765fa to 39e25866dd63641a67c110aa0b2fcd7bc0125c1e

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 3 months ago by mkoeppe

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

  • Commit changed from 39e25866dd63641a67c110aa0b2fcd7bc0125c1e to cc3aa94db91394f183100d0bea8c6babaeefe532

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

cc3aa94RealSet: Fix up docstring, add table

comment:48 Changed 3 months ago by gh-mjungmath

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

  • Commit changed from cc3aa94db91394f183100d0bea8c6babaeefe532 to cb307e1ad534c9221aa90449170a22c9a714d5da

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 3 months ago by gh-mjungmath

  • Status changed from needs_review to needs_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 3 months ago by tscrim

  • 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 3 months ago by gh-mjungmath

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

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 3 months ago by gh-mjungmath

  • Status changed from needs_review to positive_review

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

LGTM.

comment:55 Changed 3 months ago by gh-mjungmath

  • Reviewers set to Michael Jung, Eric Gourgoulhon

comment:56 Changed 3 months ago by mkoeppe

Thanks!

comment:57 Changed 3 months ago by vbraun

  • Status changed from positive_review to needs_work

PDF docs don't build

comment:58 Changed 3 months ago by git

  • Commit changed from cb307e1ad534c9221aa90449170a22c9a714d5da to 8093a4d772b6ef51164cfe95a68eb5d4e8ea5a23

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 3 months ago by mkoeppe

  • Status changed from needs_work to positive_review

Works for me after merging the lastest versions of the dependencies

comment:60 Changed 3 months ago by vbraun

  • 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 3 months ago by git

  • Commit changed from 8093a4d772b6ef51164cfe95a68eb5d4e8ea5a23 to f85db25a9c30bcd91470fda5358b3d953e470233

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 3 months ago by mkoeppe

  • Status changed from needs_work to positive_review

OK, found it, fixed it

comment:63 Changed 3 months ago by vbraun

  • 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
Note: See TracTickets for help on using tickets.