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

Consolidate ReferenceRegion, Position, etc. #601

Closed
fnothaft opened this issue Mar 1, 2015 · 9 comments
Closed

Consolidate ReferenceRegion, Position, etc. #601

fnothaft opened this issue Mar 1, 2015 · 9 comments
Milestone

Comments

@fnothaft
Copy link
Member

fnothaft commented Mar 1, 2015

We had discussed this a bit on another thread; this winds up being a consolidation of #511, #517, #592, Here's what I propose:

  1. Eliminate ReferencePosition. A ReferencePosition is just a special case of a ReferenceRegion (region with width = 1). For the cost of 1 Long field, we could eliminate a lot of duplicated code.
  2. Eliminate the *WithOrientation models. We have a Strand enum: https://github.com/bigdatagenomics/bdg-formats/blob/master/src/main/resources/avro/bdg.avdl#L720.
  3. Move ReferenceRegion to Avro. This will allow us to consolidate fields in AlignmentRecord, Variant, and NucleotideContigFragment. I think we'd keep the org.bdgenomics.adam.models.ReferenceRegion object, and consolidate an apply method, as well as the overlaps, isAdjacent, etc methods there.
  4. As proposed in ReferenceRegion shouldn't extend Ordered #511, add an Ordering for ReferenceRegion.
@fnothaft fnothaft added this to the 0.17.0 milestone Mar 1, 2015
@tdanford
Copy link
Contributor

tdanford commented Mar 1, 2015

+1 on the first two points, and the fourth. Need to think about the third?

@tdanford
Copy link
Contributor

tdanford commented Mar 1, 2015

Will this really allow us to consolidate AlignmentRecord, Variant, etc?

Wouldn't the ideal be that those (generated) classes all inherit from a common ReferenceRegion superclass? Is that possible with Avro?

(Isn't this what sparked our discussion of Thrift on the call a week or two ago?)

@fnothaft
Copy link
Member Author

fnothaft commented Mar 1, 2015

I don't think it'd be ideal to have them inherit from a common class, because they all have different implementations. E.g., variants have a single required mapping. Reads (in the fragment model, which I prefer) have multiple optional mappings. Contigs have a single optional mapping.

@fnothaft
Copy link
Member Author

fnothaft commented Mar 1, 2015

I'll open a PR against bdg-formats showing what I'm talking about.

@tdanford
Copy link
Contributor

tdanford commented Mar 1, 2015

That'd be great.

@laserson
Copy link
Contributor

laserson commented Mar 1, 2015

I agree with @tdanford. Bullet 3 seems like a more complex issue, which I'm more ok to punt on at the moment. I agree that there are advantages to having a non-Avro ReferenceRegion, so would it be confusing to also have an Avro ReferenceRegion?

@tdanford
Copy link
Contributor

tdanford commented Mar 2, 2015

Frank, can you rewrite the issue description above to use a number list rather than bullets? :-)

@fnothaft
Copy link
Member Author

fnothaft commented Mar 2, 2015

Frank, can you rewrite the issue description above to use a number list rather than bullets? :-)

Fixed!

@fnothaft
Copy link
Member Author

Closed via #624.

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 a pull request may close this issue.

3 participants