Opened 7 years ago
Closed 3 years ago
#16063 closed enhancement (fixed)
Unify the classes RealSet and UnionOfIntervals
Reported by:  pbruin  Owned by:  

Priority:  major  Milestone:  sage8.2 
Component:  calculus  Keywords:  real intervals 
Cc:  ares, cremona, robertwb, vbraun, rws, tscrim  Merged in:  
Authors:  Matthias Koeppe  Reviewers:  Ralf Stephan 
Report Upstream:  N/A  Work issues:  
Branch:  2191e2a (Commits, GitHub, GitLab)  Commit:  2191e2a5b55dff6c6bb898c3c8e5a8af70777efc 
Dependencies:  #8828, #13125  Stopgaps: 
Description
The following two classes were introduced almost simultaneously:
sage.sets.real_set.RealSet
in #13125, which implements finite unions of open/closed/semiclosed intervals;sage.schemes.elliptic_curves.height.UnionOfIntervals
in #8828, which has a more restricted scope (only unions of closed intervals).
It would be reasonable if these two classes were merged, and if all the functionality of UnionOfIntervals
that is not yet in RealSet
were moved there (e.g. scaling, computing the endpoints/boundary).
Change History (13)
comment:1 Changed 7 years ago by
 Milestone changed from sage6.2 to sage6.3
comment:2 Changed 7 years ago by
 Milestone changed from sage6.3 to sage6.4
comment:3 Changed 5 years ago by
 Branch set to u/mkoeppe/unify_the_classes_realset_and_unionofintervals
comment:4 Changed 5 years ago by
 Commit set to 63f2fc9f32081c0e17c503625374c001436d304f
comment:5 Changed 5 years ago by
 Milestone changed from sage6.4 to sage7.4
 Status changed from new to needs_review
Volker: Would you have a moment to take a look whether I'm on the right track here.
I'm wondering why the normalization of the init arguments of InternalRealInterval
are not done by a method InternalRealInterval.__classcall__
but rather by RealSet._prep
. Because of this I have to duplicate some of this normalization in InternalRealInterval.__mul__
.
comment:6 Changed 5 years ago by
 Cc rws added
comment:7 Changed 4 years ago by
 Cc tscrim added
comment:8 followup: ↓ 9 Changed 4 years ago by
Looking briefly at the code, UnionOfIntervals
is somewhat more specialized, and so it is likely is faster than the more generic (and [indirectly] in the global namespace) InternalRealInterval
. I'm not convinced that we should remove UnionOfIntervals
unless there is not a speed regression (or it is minor).
However, I am +1 for unifying the behavior of these two classes. If you feel that you are having to duplicate some of the normalization code, then it might be better to try and refactor it because of the new usecase you have.
comment:9 in reply to: ↑ 8 ; followup: ↓ 10 Changed 4 years ago by
Replying to tscrim:
Looking briefly at the code,
UnionOfIntervals
is somewhat more specialized, and so it is likely is faster than the more generic (and [indirectly] in the global namespace)InternalRealInterval
. I'm not convinced that we should removeUnionOfIntervals
unless there is not a speed regression (or it is minor).
+1 The UnionOfIntervals? was written for one specific application where timing is very important.
However, I am +1 for unifying the behavior of these two classes. If you feel that you are having to duplicate some of the normalization code, then it might be better to try and refactor it because of the new usecase you have.
comment:10 in reply to: ↑ 9 Changed 4 years ago by
Replying to cremona:
Replying to tscrim:
Looking briefly at the code,
UnionOfIntervals
is somewhat more specialized, and so it is likely is faster than the more generic (and [indirectly] in the global namespace)InternalRealInterval
. I'm not convinced that we should removeUnionOfIntervals
unless there is not a speed regression (or it is minor).+1 The UnionOfIntervals? was written for one specific application where timing is very important.
Do you have a benchmark that I can run without having to know anything about your application?
comment:11 Changed 4 years ago by
 Commit changed from 63f2fc9f32081c0e17c503625374c001436d304f to 2191e2a5b55dff6c6bb898c3c8e5a8af70777efc
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
2191e2a  RealSet, InternalRealInterval: Multiplication scales

comment:12 Changed 3 years ago by
 Milestone changed from sage7.4 to sage8.2
 Reviewers set to Ralf Stephan
 Status changed from needs_review to positive_review
 Type changed from task to enhancement
I think this is needed, it looks fine, and a patchbot is green.
comment:13 Changed 3 years ago by
 Branch changed from u/mkoeppe/unify_the_classes_realset_and_unionofintervals to 2191e2a5b55dff6c6bb898c3c8e5a8af70777efc
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
RealSet, InternalRealInterval: Multiplication scales