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: sage-8.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:

Status badges

Description

The following two classes were introduced almost simultaneously:

  • sage.sets.real_set.RealSet in #13125, which implements finite unions of open/closed/semi-closed 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 vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:2 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:3 Changed 5 years ago by mkoeppe

  • Branch set to u/mkoeppe/unify_the_classes_realset_and_unionofintervals

comment:4 Changed 5 years ago by mkoeppe

  • Authors set to Matthias Koeppe
  • Commit set to 63f2fc9f32081c0e17c503625374c001436d304f

New commits:

63f2fc9RealSet, InternalRealInterval: Multiplication scales

comment:5 Changed 5 years ago by mkoeppe

  • Milestone changed from sage-6.4 to sage-7.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 mkoeppe

  • Cc rws added

comment:7 Changed 4 years ago by mkoeppe

  • Cc tscrim added

comment:8 follow-up: Changed 4 years ago by 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 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 use-case you have.

comment:9 in reply to: ↑ 8 ; follow-up: Changed 4 years ago by 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 remove UnionOfIntervals 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 use-case you have.

comment:10 in reply to: ↑ 9 Changed 4 years ago by mkoeppe

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 remove UnionOfIntervals 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 git

  • Commit changed from 63f2fc9f32081c0e17c503625374c001436d304f to 2191e2a5b55dff6c6bb898c3c8e5a8af70777efc

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

2191e2aRealSet, InternalRealInterval: Multiplication scales

comment:12 Changed 3 years ago by rws

  • Milestone changed from sage-7.4 to sage-8.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 vbraun

  • Branch changed from u/mkoeppe/unify_the_classes_realset_and_unionofintervals to 2191e2a5b55dff6c6bb898c3c8e5a8af70777efc
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.