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

Populating the DB SNP validation status field properly #5046

Merged
merged 6 commits into from
Aug 16, 2018

Conversation

LeeTL1220
Copy link
Contributor

@codecov-io
Copy link

codecov-io commented Jul 23, 2018

Codecov Report

Merging #5046 into master will decrease coverage by 1.008%.
The diff coverage is 92.188%.

@@               Coverage Diff               @@
##              master     #5046       +/-   ##
===============================================
- Coverage     86.382%   85.374%   -1.008%     
- Complexity     28674     28718       +44     
===============================================
  Files           1785      1791        +6     
  Lines         132713    134756     +2043     
  Branches       14766     15131      +365     
===============================================
+ Hits          114640    115047      +407     
- Misses         12746     14348     +1602     
- Partials        5327      5361       +34
Impacted Files Coverage Δ Complexity Δ
...tils/annotatedinterval/AnnotatedIntervalUtils.java 82.979% <ø> (ø) 10 <0> (ø) ⬇️
...er/tools/funcotator/FuncotatorIntegrationTest.java 84.305% <100%> (+0.932%) 72 <2> (+3) ⬆️
...itute/hellbender/tools/funcotator/Funcotation.java 55.556% <100%> (+22.222%) 5 <2> (+2) ⬆️
...ncotator/mafOutput/MafOutputRendererConstants.java 99.02% <100%> (ø) 1 <0> (ø) ⬇️
.../tools/funcotator/mafOutput/MafOutputRenderer.java 95.98% <100%> (+0.062%) 61 <1> (+2) ⬆️
...cotator/dataSources/vcf/VcfFuncotationFactory.java 87.075% <76%> (-2.525%) 48 <6> (+4)
...cotator/mafOutput/CustomMafFuncotationCreator.java 91.429% <93.75%> (+1.429%) 23 <6> (+6) ⬆️
...mafOutput/CustomMafFuncotationCreatorUnitTest.java 97.802% <94.118%> (-2.198%) 12 <6> (+6)
...ender/tools/spark/sv/utils/SVIntervalUnitTest.java 18.519% <0%> (-64.815%) 2% <0%> (ø)
...on/FindBreakpointEvidenceSparkIntegrationTest.java 54.737% <0%> (-45.263%) 7% <0%> (+1%)
... and 26 more

@LeeTL1220
Copy link
Contributor Author

@jonn-smith Looks like the travis failures are transient...

@LeeTL1220
Copy link
Contributor Author

@jonn-smith Can you review this?

Copy link
Collaborator

@jonn-smith jonn-smith left a comment

Choose a reason for hiding this comment

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

One question and minor request.

if (doesRegion1ContainAnnotation && doesRegion2ContainAnnotation) {

// Both regions contain an annotation and presumably these are of different values.
return conflictFunction.apply(funcotation1.getField(fieldName),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we do anything differently if the values of the funcotations are the same?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It already does nothing. Just takes the value. I think this is the correct behavior. No action.


import static org.broadinstitute.hellbender.tools.funcotator.mafOutput.MafOutputRendererConstants.MAF_RENDERING_DATASOURCE_DUMMY_NAME;
import static org.broadinstitute.hellbender.tools.funcotator.mafOutput.MafOutputRendererConstants.*;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you remove the static import and reference the constants by full name instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@jonn-smith jonn-smith assigned LeeTL1220 and unassigned jonn-smith Aug 15, 2018
@LeeTL1220
Copy link
Contributor Author

@jonn-smith Can I merge if the tests pass?

@LeeTL1220
Copy link
Contributor Author

Travis error is transient.

@LeeTL1220 LeeTL1220 merged commit be42b23 into master Aug 16, 2018
@LeeTL1220 LeeTL1220 deleted the ll_4985_dbSNP_MAF branch August 16, 2018 00:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants