Opened 12 months ago

Closed 11 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 12 months ago by gh-mjungmath

  • Description modified (diff)

comment:2 Changed 12 months ago by gh-mjungmath

  • Branch set to u/gh-mjungmath/subintervals_of_openinterval_and_uniquerepresentation

comment:3 Changed 12 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 12 months ago by tscrim

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

comment:5 Changed 12 months ago by tscrim

Also TEST: -> TESTS::.

comment:6 Changed 12 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 12 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 12 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 12 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 12 months ago by mkoeppe

That's now #30832

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

Replying to mkoeppe:

That's now #30832

+1

comment:12 Changed 12 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 12 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 12 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 12 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 12 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 12 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 12 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 12 months ago by tscrim

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

Thanks. LGTM.

comment:20 Changed 12 months ago by gh-mjungmath

Thanks for the review.

comment:21 Changed 11 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.