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

Simplify BedToIntervalList by not reimplementing coordinate conversion #1292

Merged
merged 2 commits into from
Mar 12, 2019

Conversation

lbergelson
Copy link
Member

Description

  • BedToIntervalList spends a lot of comments complaining about coordinate conversions after explicitly setting the codec to not perform the conversion from [0,..) to [1,..]
  • Simplified by just using the baked in conversion instead of disabling it.

Maybe I'm missing something clever that it's doing here...


Checklist (never delete this)

Never delete this, it is our record that procedure was followed. If you find that for whatever reason one of the checklist points doesn't apply to your PR, you can leave it unchecked but please add an explanation below.

Content

  • Added or modified tests to cover changes and any new functionality
  • Edited the README / documentation (if applicable)
  • All tests passing on Travis

Review

  • Final thumbs-up from reviewer
  • Rebase, squash and reword as applicable

For more detailed guidelines, see https://github.com/broadinstitute/picard/wiki/Guidelines-for-pull-requests

* BedToIntervalList spends a lot of comments complaining about coordinate conversions after explicitely setting the codec to not perform the conversion from [0,..) to [1,..]
* Simplified by just using the baked in conversion instead of disabling it.
@coveralls
Copy link

coveralls commented Mar 7, 2019

Coverage Status

Coverage increased (+0.04%) to 81.537% when pulling eddfc69 on lb_remove_extraneous_conversion_in_bedtointerval into 21ad30b on master.

* so it returns 1-based starts. Ugh. Set to zero.
*/
final FeatureReader<BEDFeature> bedReader = AbstractFeatureReader.getFeatureReader(INPUT.getAbsolutePath(), new BEDCodec(BEDCodec.StartOffset.ZERO), false);
final FeatureReader<BEDFeature> bedReader = AbstractFeatureReader.getFeatureReader(INPUT.getAbsolutePath(), new BEDCodec(), false);
Copy link
Contributor

Choose a reason for hiding this comment

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

will this be able to read a bed that has position 0?

Copy link
Contributor

Choose a reason for hiding this comment

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

(the tests are not very good...they just test that the dictionary is right and that the program didn't crash...)

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question.. The tests do actually test the output interval see testBedToIntervalListDataProvider, that's how we noticed that there was a bug with merging 0-length intervals.

With some experimentation: 0 intervals that start at 0 work, 0-length intervals that start as 0 have never worked... I added a fix and a new test.

To be complete we might want to allow the weird edge case of the zero-length interval AFTER the end of the contig... My guess is that doesn't work either... GATK will almost certainly explode if it sees one of those as well.

Copy link
Contributor

@yfarjoun yfarjoun left a comment

Choose a reason for hiding this comment

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

thanks.

@lbergelson lbergelson merged commit 1531278 into master Mar 12, 2019
@lbergelson lbergelson deleted the lb_remove_extraneous_conversion_in_bedtointerval branch March 12, 2019 17:06
gbggrant pushed a commit that referenced this pull request Apr 25, 2019
#1292)

* Simplify BedToIntervalList by not reimplementing coordiant conversion

* BedToIntervalList spends a lot of comments complaining about coordinate conversions after explicitely setting the codec to not perform the conversion from [0,..) to [1,..]
* Simplified by just using the baked in conversion instead of disabling it.
* add more tests and fix for zero-length edge case at the start of the contig
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.

3 participants