Add Interval class #114
Add Interval class #114
Conversation
|
Just letting you know I'll pick this up for review. |
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
|
|
||
| part of quiver.compare; |
There was a problem hiding this comment.
I think rather than quiver.compare, we might just want these in quiver.core (with your other compare PR). Another place for Interval could be quiver.math
There was a problem hiding this comment.
Agreed. Do you prefer quiver.core or quiver.math? I have a slight preference for quiver.math, considering we may want an IntervalTree or similar in the future.
There was a problem hiding this comment.
Went with quiver.math, it really is a math concept, the Bound class would feel out of place in quiver.core, and it has potential to expand in the future to e.g. multi-dimensional Intervals and IntervalTrees.
|
I was chatting w/ Chris about this, so added a few comments :) |
|
@jtmcdole @justinfagnani comments addressed, PTAL |
| /// Whether `this` [contains] any values. | ||
| bool get isEmpty { | ||
| if (lower == null || upper == null || Comparable.compare(lower, upper) < 0) return false; | ||
| return !(lowerClosed && upperClosed); |
There was a problem hiding this comment.
I would write this as return !isClosed.
|
Are you following this up with an interval tree ;) Note: I have an interval tree that I can adapt from TreeSet (it is the same code as treeset with a max subtree value at each node and some extra features for searching intervals). |
|
IntervalTree is all yours :). Hmm, I guess that would be in quiver.collections? In which case, maybe Interval should be in quiver.core after all? Or maybe everything Interval-related should go in a separate quiver.intervals library? Or a separate |
|
IntervalTree would go in collections, but when I think of "Hey, I want an interval", I don't think I should include collections. |
|
Leaving Interval/Bound in quiver.math for now. My main concern with moving to quiver.core is Bound feels out of place there. We could probably remove Bound if we really want Interval in quiver.core. |
|
John and I talked about this the other day, and one of the main arguments against quiver.math was that there are existing math libraries, and math is a huge domain that could have its own quiver-like package. I don't think we want to grow a significant math library in quiver, and don't want Interval to sit alone there either. Consider that dart:core has DateTime and Duration instead of a full-featured dart:time library, and I think this fits in well. |
|
Agreed. Moved to |
| class Bound<T> { | ||
|
|
||
| /// The boundary value. | ||
| final T value; |
There was a problem hiding this comment.
Is there a way to differentiate upper and lower unbounded bounds? If there was, then Bound could be comparable.
|
Going to publish this as its own package. |
This still needs docs and a bit of formatting cleanup, but the tests are extensive, so it should be ready for general feedback.
Main inspirations for this were:
http://docs.guava-libraries.googlecode.com/git/javadoc/com/google/common/collect/Range.html
http://en.wikipedia.org/wiki/Interval_(mathematics)
I put it in
quiver.comparesince I also want to add stuff from #60 there.quiver.comparablescould also work, but that might not make as much sense if Interval or other functionality works with arbitraryComparators instead of requingComparables.quiver.mathis another option.I deprecated
quiver.iterables.extent(iterable)in favor ofnew Interval.span(iterable).