Opened 9 months ago
Closed 8 months ago
#30830 closed defect (fixed)
Subintervals of OpenInterval and UniqueRepresentation
Reported by:  ghmjungmath  Owned by:  

Priority:  major  Milestone:  sage9.3 
Component:  manifolds  Keywords:  
Cc:  egourgoulhon, tscrim, ghtobiasdiez  Merged in:  
Authors:  Michael Jung  Reviewers:  Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  200942c (Commits, GitHub, GitLab)  Commit:  200942cf97f137e880ea21b76171ab71a45f33d8 
Dependencies:  #30799  Stopgaps: 
Description (last modified by )
At the moment, we have the following behavior:
sage: I = OpenInterval(0,2) sage: J = OpenInterval(0,1, ambient_interval=I, coordinate='t') sage: I.open_interval(0,1) Traceback (most recent call last) ... ValueError: the name '(0, 1)' is already used for another subset of the Real interval (0, 2)
Even though the use of OpenInterval(0,1, ambient_interval=I)
is not intended, this is still a blind spot.
The reason for this behavior comes from the UniqueRepresentation
and how the subintervals are constructed.
I propose a fix using __classcall_private__
.
Change History (21)
comment:1 Changed 9 months ago by
 Description modified (diff)
comment:2 Changed 9 months ago by
 Branch set to u/ghmjungmath/subintervals_of_openinterval_and_uniquerepresentation
comment:3 Changed 9 months ago by
 Commit set to 25e96e9ec7f5dc7ac409b8a1e17674ae8e722176
 Status changed from new to needs_review
comment:4 Changed 9 months ago by
You have a doctest failure in continuous_map
(see the patchbot). Otherwise LGTM.
comment:5 Changed 9 months ago by
Also TEST:
> TESTS::
.
comment:6 Changed 9 months ago by
 Commit changed from 25e96e9ec7f5dc7ac409b8a1e17674ae8e722176 to aab0f1428ce0d68fd49af6891190e6b892a1aede
Branch pushed to git repo; I updated commit sha1. New commits:
aab0f14  Trac #30830: utilize UniqueRepresentation instead

comment:7 Changed 9 months ago by
I think, this approach is slighly better because it is closer to the original behavior. Now, the test in continuous_map.py
should pass.
comment:8 followup: ↓ 9 Changed 9 months ago by
Side remark: It would be nice to connect OpenInterval
also to RealSet
(not on this ticket)...
comment:9 in reply to: ↑ 8 Changed 9 months ago by
Replying to mkoeppe:
Side remark: It would be nice to connect
OpenInterval
also toRealSet
(not on this ticket)...
Nice! Would you take the honor to open the corresponding ticket?
comment:10 followup: ↓ 11 Changed 9 months ago by
That's now #30832
comment:11 in reply to: ↑ 10 Changed 9 months ago by
comment:12 Changed 9 months ago by
With this change, you can create two distinct intervals with the same (latex) name by passing the resulting values if None
is given (e.g., name='R'
). While this is me being deliberately evil, it is something I want you to consider and if you are okay with this possible situation happening.
comment:13 followup: ↓ 14 Changed 9 months ago by
You mean
sage: R = RealLine(); R Real number line R sage: R1 = RealLine(name='R'); R1 Real number line R sage: R is R1 False
Right?
comment:14 in reply to: ↑ 13 Changed 9 months ago by
Replying to ghmjungmath:
You mean
sage: R = RealLine(); R Real number line R sage: R1 = RealLine(name='R'); R1 Real number line R sage: R is R1 FalseRight?
Yes, exactly.
comment:15 Changed 9 months ago by
 Dependencies set to #30799
This should do, but it doesn't. Adding __classcall__
to RealLine
doesn't help either.
comment:16 Changed 9 months ago by
 Commit changed from aab0f1428ce0d68fd49af6891190e6b892a1aede to 224ab518ee28dbc396c25de5995fa626ea6e00cd
comment:17 Changed 9 months ago by
 Commit changed from 224ab518ee28dbc396c25de5995fa626ea6e00cd to 200942cf97f137e880ea21b76171ab71a45f33d8
Branch pushed to git repo; I updated commit sha1. New commits:
200942c  Trac #30830: same name yields same instance

comment:18 Changed 9 months ago by
This works now.
I think that OpenInterval
should silently return a RealLine
instance if lower == minus_infinity
and upper == infinity
. As Eric pointed out, the Real line is a fully determined mathematical object, and the behaviour here should reflect that.
But this should be part of another ticket.
comment:19 Changed 9 months ago by
 Reviewers set to Travis Scrimshaw
 Status changed from needs_review to positive_review
Thanks. LGTM.
comment:20 Changed 9 months ago by
Thanks for the review.
comment:21 Changed 8 months ago by
 Branch changed from u/ghmjungmath/subintervals_of_openinterval_and_uniquerepresentation to 200942cf97f137e880ea21b76171ab71a45f33d8
 Resolution set to fixed
 Status changed from positive_review to closed
Ready for review.
New commits:
Trac #30830: add __classcall_private__ to OpenInterval