Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RealSet: Extend constructors so that they can build manifold objects #31881

Closed
mkoeppe opened this issue May 30, 2021 · 81 comments
Closed

RealSet: Extend constructors so that they can build manifold objects #31881

mkoeppe opened this issue May 30, 2021 · 81 comments

Comments

@mkoeppe
Copy link
Member

mkoeppe commented May 30, 2021

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.

Depends on #31688
Depends on #32089
Depends on #30473

CC: @egourgoulhon @tscrim @mjungmath

Component: manifolds

Author: Matthias Koeppe

Branch/Commit: f85db25

Reviewer: Michael Jung, Eric Gourgoulhon

Issue created by migration from https://trac.sagemath.org/ticket/31881

@mkoeppe mkoeppe added this to the sage-9.4 milestone May 30, 2021
@mkoeppe

This comment has been minimized.

@egourgoulhon
Copy link
Member

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!

@mkoeppe
Copy link
Member Author

mkoeppe commented Jun 28, 2021

comment:3

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.

@mkoeppe
Copy link
Member Author

mkoeppe commented Jul 6, 2021

Dependencies: #31688

@mkoeppe
Copy link
Member Author

mkoeppe commented Jul 7, 2021

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 7, 2021

Commit: 9db24e2

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 7, 2021

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

9db24e2RealSet: Handle keywords arguments structure, name

@mkoeppe
Copy link
Member Author

mkoeppe commented Jul 7, 2021

Changed dependencies from #31688 to #31688, #32089

@mkoeppe
Copy link
Member Author

mkoeppe commented Jul 7, 2021

Changed dependencies from #31688, #32089 to #31688, #32089, #30473

@mkoeppe
Copy link
Member Author

mkoeppe commented Jul 7, 2021

Author: Matthias Koeppe

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 7, 2021

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 7, 2021

Changed commit from 9db24e2 to 6f3a9cc

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 7, 2021

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 7, 2021

Changed commit from 6f3a9cc to 5a31e05

@mkoeppe

This comment has been minimized.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 7, 2021

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

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 7, 2021

Changed commit from 5a31e05 to 766b141

@mkoeppe

This comment has been minimized.

@mjungmath
Copy link

comment:15

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.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 7, 2021

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 7, 2021

Changed commit from 766b141 to d6055c4

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 8, 2021

Changed commit from d6055c4 to ffe18b0

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 8, 2021

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

@mkoeppe
Copy link
Member Author

mkoeppe commented Jul 8, 2021

comment:18

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

@mjungmath
Copy link

comment:52

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

@tscrim
Copy link
Collaborator

tscrim commented Jul 13, 2021

comment:53

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.

@mjungmath
Copy link

comment:54

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

LGTM.

@mjungmath
Copy link

Reviewer: Michael Jung, Eric Gourgoulhon

@mkoeppe
Copy link
Member Author

mkoeppe commented Jul 13, 2021

comment:56

Thanks!

@vbraun
Copy link
Member

vbraun commented Jul 23, 2021

comment:57

PDF docs don't build

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 23, 2021

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 23, 2021

Changed commit from cb307e1 to 8093a4d

@mkoeppe
Copy link
Member Author

mkoeppe commented Jul 23, 2021

comment:59

Works for me after merging the lastest versions of the dependencies

@vbraun
Copy link
Member

vbraun commented Jul 24, 2021

comment:60

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 24, 2021

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 24, 2021

Changed commit from 8093a4d to f85db25

@mkoeppe
Copy link
Member Author

mkoeppe commented Jul 24, 2021

comment:62

OK, found it, fixed it

@vbraun
Copy link
Member

vbraun commented Jul 25, 2021

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants