Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[hail] Add interval reduction and intersection functions #6073

Merged
merged 2 commits into from May 8, 2019

Conversation

tpoterba
Copy link
Contributor

@tpoterba tpoterba commented May 8, 2019

I want to use this in #5979

Copy link
Collaborator

@patrick-schultz patrick-schultz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think RVDPartitioner already has all this functionality. RVDPartitioner is maybe a bad name for that class, it's really the place for methods manipulating and creating ordered lists of disjoint intervals. The allowedOverlap stuff is a bit confusing; it might be worth adding some convenience methods for the simple case of length 1 keys.

@@ -226,4 +226,52 @@ object Interval {
}
}
}

def sortAndReduce(xs: Array[Interval], o: ExtendedOrdering): Array[Interval] = {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks equivalent to RVDPartitioner.union(ktype, xs, allowedOverlap = ktype.size)

}

// assumes that both `x1` and `x2` are sorted and fully reduced.
def intersection(x1: Array[Interval], x2: Array[Interval], o: ExtendedOrdering): Array[Interval] = {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this duplicates RVDPartitioner.intersect.

@tpoterba
Copy link
Contributor Author

tpoterba commented May 8, 2019

Huh, you're totally right. I was surprised I couldn't find these methods.

RVDPartitioner only works with structs, though. I'll change RVDPartitioner to call these methods.

@tpoterba
Copy link
Contributor Author

tpoterba commented May 8, 2019

The RVDPartitioner union function contains coarsening logic, so I'm not going to abstract that one right now.

@danking danking merged commit aff0a36 into hail-is:master May 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants