Conversation
nleroy917
left a comment
There was a problem hiding this comment.
Just really a small question inside MultiChromOverlapper
| /// **Note:** All operations produce regions with `rest: None`. Metadata from | ||
| /// the `rest` field of input regions is not preserved — operations like `reduce()` | ||
| /// merge multiple regions, so there is no unambiguous `rest` to carry forward. | ||
| pub trait IntervalRanges { |
There was a problem hiding this comment.
Just curious what the motivation was to make this a trait? Is it implemented elsewhere?
There was a problem hiding this comment.
I didn't make the trait... this PR isn't introducing the trait, it was there.
Looks like @sanghoonio created the IntervalRanges trait in commit 6f465f2 on Feb 18, with the initial 5 GenomicRanges operations, and followed up with a second commit on Feb 20
There was a problem hiding this comment.
I believe it doesn't make sense for this to be a trait, at this point, but I'll let @sanghoonio respond.
| return RegionSet::from(Vec::<Region>::new()); | ||
| } | ||
|
|
||
| let index = other.clone().into_multi_chrom_overlapper(OverlapperType::AIList); |
There was a problem hiding this comment.
Just a thought... this operation can be expensive since you're building the index. Does it make more sense for this function to take as input a MultiChromOverlapper? That way if you are performing many intersect calls, you can build the index once and pass in by reference?
I guess just proposing to decouple the indexing building from the intersection?
There was a problem hiding this comment.
yeah this is a legitimate concern. but it points at something more fundamental. we need to re-evaluate the whole trait/regionset/MCO relationships, which I can't really tackle at this point. So will address this in the future.
…ll to avoid collision
Working on improving performance of bedshift; need some work on gtars guts to python.