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

[ADAM-1365] Apply validation stringency to reads on missing contigs when MD tagging #1366

Merged

Conversation

fnothaft
Copy link
Member

Resolves #1365. If a read is mapped to a contig that is missing from the broadcasted fragment map, we look at the user's provided validation stringency before throwing an exception.

…hen MD tagging

Resolves bigdatagenomics#1365. If a read is mapped to a contig that is missing from the broadcasted
fragment map, we look at the user's provided validation stringency before throwing
an exception.
@fnothaft fnothaft added this to the 0.22.0 milestone Jan 23, 2017
.extract(ReferenceRegion.unstranded(read)))
} catch {
case t: Throwable => {
if (validationStringency == ValidationStringency.STRICT) {
Copy link
Member

Choose a reason for hiding this comment

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

The more we use ValidationStringency outside of forwarding to htsjdk, the less I like using their enum.

I know this isn't the place; what do you think of an ADAM-specific enum that maps to ValidationStringency where necessary? We could use the opportunity to come up with a better name, since it isn't really about validation, it is more about error reporting.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm +0. I don't love ValidationStringency, but I'm a bit too lazy to deconvolve it from our codebase. I guess I'd say that it is on the sorted list of refactors that I'd like to do, but lower down on the list. That being said, it's probably not that much work. CC @ryan-williams for thoughts as well.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah that's where I'm at too. If I had a better name ready, I'd do it.

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/ADAM-prb/1750/
Test PASSed.

@heuermh heuermh merged commit c0ed2b2 into bigdatagenomics:master Jan 23, 2017
@heuermh
Copy link
Member

heuermh commented Jan 23, 2017

Thank you, @fnothaft!

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