Opened 22 months ago
Closed 21 months ago
#30830 closed defect (fixed)
Subintervals of OpenInterval and UniqueRepresentation
Reported by: | gh-mjungmath | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-9.3 |
Component: | manifolds | Keywords: | |
Cc: | egourgoulhon, tscrim, gh-tobiasdiez | 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 22 months ago by
- Description modified (diff)
comment:2 Changed 22 months ago by
- Branch set to u/gh-mjungmath/subintervals_of_openinterval_and_uniquerepresentation
comment:3 Changed 22 months ago by
- Commit set to 25e96e9ec7f5dc7ac409b8a1e17674ae8e722176
- Status changed from new to needs_review
comment:4 Changed 22 months ago by
You have a doctest failure in continuous_map
(see the patchbot). Otherwise LGTM.
comment:5 Changed 22 months ago by
Also TEST:
-> TESTS::
.
comment:6 Changed 22 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 22 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 follow-up: ↓ 9 Changed 22 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 22 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 follow-up: ↓ 11 Changed 22 months ago by
That's now #30832
comment:11 in reply to: ↑ 10 Changed 22 months ago by
comment:12 Changed 22 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 follow-up: ↓ 14 Changed 22 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 22 months ago by
Replying to gh-mjungmath:
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 22 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 22 months ago by
- Commit changed from aab0f1428ce0d68fd49af6891190e6b892a1aede to 224ab518ee28dbc396c25de5995fa626ea6e00cd
comment:17 Changed 22 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 22 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 22 months ago by
- Reviewers set to Travis Scrimshaw
- Status changed from needs_review to positive_review
Thanks. LGTM.
comment:20 Changed 22 months ago by
Thanks for the review.
comment:21 Changed 21 months ago by
- Branch changed from u/gh-mjungmath/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