Add Predecessible and methods to Interval #262
Conversation
|
closes #257 |
| implicit def numPrev[N: Numeric]: Predecessible[N] = new NumericPredecessible[N] | ||
| } | ||
|
|
||
| class NumericPredecessible[@specialized(Int,Long,Float,Double) T:Numeric] extends Predecessible[T] { |
There was a problem hiding this comment.
remove the specialized. Option isn't, so what's the point?
| def iteratePrev[T](first: T)(implicit p: Predecessible[T]): Iterable[T] = | ||
| p.iteratePrev(first) | ||
|
|
||
| implicit def intergalPrev[N: Integral]: Predecessible[N] = new IntegralPredecessible[N] |
There was a problem hiding this comment.
typo: intergal. We need spell checkers that understand camelCase and under_scores.
| * it this way, it does not mean it is empty or universe, etc... (there | ||
| * are other cases). | ||
| */ | ||
| def toLeftClosedRightOpen(implicit s: Successible[T]): Option[(T, T)] = |
There was a problem hiding this comment.
How to you imagine this being used? This seems like a terribly specific method. I guess I don't want to have 4 methods for all of the kinds, and would rather thing of a more general way to let people transforms intervals into forms that they wnt
There was a problem hiding this comment.
Well, we actually have a lot of interval parsing code in summingbird that is a pain given how general Intervals can be. This was for the common cases of finite interval that can be expressed as >= low, < high, which has the benefit of being easy to split: given low < h1 < high, then you can break this into two adjacent intervals: (low, h1), (h1, high).
Especially when converting to existing code that uses ranges like this, this method can be useful.
There was a problem hiding this comment.
The issues I've seen are around super-intervals (if that's the right word), e.g., batches being a superset of timestamps. Would it make sense to optimize around this?
For example, something like:
def toIntervalOf[U](implicit mapping: Injection[T, Interval[U]]): Interval[U]Maybe it should be abstracted a different way though, to more simply handle cases like going from timestamp back to batch.
There was a problem hiding this comment.
This is an interesting case. So far, we have this mapNonDecreasing on intervals, but your point is nice: often a value in one case is a interval in the other. Such as: Injection[BatchID, Interval[Timestamp]].
If [B1, B2), that means that you have [least(time(B1)), least(time(B2)))
Similarly:
[B1, B2] => [least(time(B1)), greatest(time(B2))]
(B1, B2) => [greatest(time(B1)), least(time(B2)))
etc…
I think this is a useful pattern. Consider The truncation of Injection[Int, Interval[Double]] 1 => [1.0, 2.0) for instance. Though, really we might want to loosen it to Injection[Interval[Int], Interval[Double]], this is what it sounds like you were saying with supersets: If and interval is a super-interval of another, it means there is an Injection[Interval[T], Interval[U]] right?
I'd rather follow this up with more code moved from summingbird into this interval, successible, set of stuff. Can we do this in a next PR?
There was a problem hiding this comment.
Perhaps the proper abstraction is to have a "smallest containing interval" and "largest contained interval", if we want to also cleanly handle cases where interval edges don't necessarily line up, such as when going from Timestamp to BatchID.
Anyway, separate PRs is better.
|
I am digging this, Oscar. |
|
LGTM, if no objections pop up I'll merge in the morning |
|
@johnynek giving it a final look and will merge shortly |
Add Predecessible and methods to Interval
This also improves the tests on Interval by actually generating Intersections (whoops), which uncovered a bug when doing Intersection.intersect with anything other than another Intersection.