Opened 9 months ago

Closed 8 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:

Status badges

Description (last modified by gh-mjungmath)

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 gh-mjungmath

  • Description modified (diff)

comment:2 Changed 9 months ago by gh-mjungmath

  • Branch set to u/gh-mjungmath/subintervals_of_openinterval_and_uniquerepresentation

comment:3 Changed 9 months ago by gh-mjungmath

  • Authors set to Michael Jung
  • Commit set to 25e96e9ec7f5dc7ac409b8a1e17674ae8e722176
  • Status changed from new to needs_review

Ready for review.

New commits:

​25e96e9Trac #30830: add __classcall_private__ to OpenInterval

comment:4 Changed 9 months ago by tscrim

You have a doctest failure in continuous_map (see the patchbot). Otherwise LGTM.

comment:5 Changed 9 months ago by tscrim

Also TEST: -> TESTS::.

comment:6 Changed 9 months ago by git

  • Commit changed from 25e96e9ec7f5dc7ac409b8a1e17674ae8e722176 to aab0f1428ce0d68fd49af6891190e6b892a1aede

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

​aab0f14Trac #30830: utilize UniqueRepresentation instead

comment:7 Changed 9 months ago by gh-mjungmath

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: Changed 9 months ago by mkoeppe

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 gh-mjungmath

Replying to mkoeppe:

Side remark: It would be nice to connect OpenInterval also to RealSet (not on this ticket)...

Nice! Would you take the honor to open the corresponding ticket?

comment:10 follow-up: Changed 9 months ago by mkoeppe

That's now #30832

comment:11 in reply to: ↑ 10 Changed 9 months ago by gh-mjungmath

Replying to mkoeppe:

That's now #30832

+1

comment:12 Changed 9 months ago by tscrim

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: Changed 9 months ago by 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                                                                   
False

Right?

comment:14 in reply to: ↑ 13 Changed 9 months ago by tscrim

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                                                                   
False

Right?

Yes, exactly.

comment:15 Changed 9 months ago by gh-mjungmath

  • 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 git

  • Commit changed from aab0f1428ce0d68fd49af6891190e6b892a1aede to 224ab518ee28dbc396c25de5995fa626ea6e00cd

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

​8d2f44dTrac #30799: collect manifolds examples in 'examples' + lazy_import for catalog
​224ab51Trac #30830: add __classcall_private__ to OpenInterval

comment:17 Changed 9 months ago by git

  • Commit changed from 224ab518ee28dbc396c25de5995fa626ea6e00cd to 200942cf97f137e880ea21b76171ab71a45f33d8

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

​200942cTrac #30830: same name yields same instance

comment:18 Changed 9 months ago by gh-mjungmath

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 tscrim

  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to positive_review

Thanks. LGTM.

comment:20 Changed 9 months ago by gh-mjungmath

Thanks for the review.

comment:21 Changed 8 months ago by vbraun

  • Branch changed from u/gh-mjungmath/subintervals_of_openinterval_and_uniquerepresentation to 200942cf97f137e880ea21b76171ab71a45f33d8
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.