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

PrintBGZFBlockInformation: remove file extension check so that we can accept, eg., bams #5801

Merged
merged 2 commits into from
Mar 27, 2019

Conversation

droazen
Copy link
Collaborator

@droazen droazen commented Mar 15, 2019

The file extension check in this tool was redundant, since there is a subsequent check
that the input file is in BGZF format.

Resolves #5800

… accept, eg., bams

The file extension check in this tool was redundant, since there is a subsequent check
that the input file is in BGZF format.

Resolves #5800
@droazen
Copy link
Collaborator Author

droazen commented Mar 15, 2019

@jamesemery please review (@kvg ran into this issue yesterday)

@codecov-io
Copy link

codecov-io commented Mar 15, 2019

Codecov Report

Merging #5801 into master will decrease coverage by 1.419%.
The diff coverage is 88.235%.

@@               Coverage Diff               @@
##              master     #5801       +/-   ##
===============================================
- Coverage     87.002%   85.583%   -1.419%     
+ Complexity     32111     31394      -717     
===============================================
  Files           1974      1977        +3     
  Lines         147246    147095      -151     
  Branches       16217     16170       -47     
===============================================
- Hits          128107    125888     -2219     
- Misses         13233     15211     +1978     
- Partials        5906      5996       +90
Impacted Files Coverage Δ Complexity Δ
...te/hellbender/tools/PrintBGZFBlockInformation.java 82.292% <100%> (+2.494%) 21 <0> (ø) ⬇️
...ools/PrintBGZFBlockInformationIntegrationTest.java 91.045% <87.5%> (-1.112%) 9 <2> (+2)
...ect/filtering/ChimericOriginalAlignmentFilter.java 0% <0%> (-100%) 0% <0%> (-8%)
...ct/CreateSomaticPanelOfNormalsIntegrationTest.java 3.448% <0%> (-94.253%) 2% <0%> (-8%)
...ls/walkers/mutect/CreateSomaticPanelOfNormals.java 0% <0%> (-94.03%) 0% <0%> (-22%)
...aplotypecaller/HaplotypeCallerIntegrationTest.java 0.431% <0%> (-87.5%) 2% <0%> (-85%)
...alkers/mutect/SomaticReferenceConfidenceModel.java 12.5% <0%> (-84.375%) 1% <0%> (-7%)
...r/tools/walkers/mutect/Mutect2IntegrationTest.java 5.527% <0%> (-82.059%) 2% <0%> (-87%)
...ls/genomicsdb/GenomicsDBImportIntegrationTest.java 9.313% <0%> (-79.157%) 2% <0%> (-78%)
...walkers/mutect/filtering/NormalArtifactFilter.java 21.739% <0%> (-73.913%) 5% <0%> (-3%)
... and 147 more

Copy link
Collaborator

@jamesemery jamesemery left a comment

Choose a reason for hiding this comment

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

Mostly some pedantic documentation requests.

@@ -71,11 +71,6 @@ protected void onStartup() {
throw new UserException.CouldNotReadInputFile("File " + bgzfPathString + " does not exist");
}

if ( ! IOUtil.hasBlockCompressedExtension(bgzfPathString) ) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe you should amend the below statement "File is not a valid BGZF file. Could possibly be a regular GZIP file?" to better reflect that the user may have provided any file under the sun.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would also explicitly add a reference to the javadocs for this tool that mentions the bam use case as it sounds incredibly useful to people.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done and done

@@ -100,4 +100,20 @@ public void testRegularGzipFile() throws IOException {
};
runCommandLine(args);
}

/* Make sure that we can handle a standard BAM file */
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe add a test to ensure there is a nice exception when the user provides a non-BGZF file (maybe a VCF).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@jamesemery jamesemery assigned droazen and unassigned jamesemery Mar 18, 2019
@droazen droazen assigned jamesemery and unassigned droazen Mar 27, 2019
@droazen
Copy link
Collaborator Author

droazen commented Mar 27, 2019

@jamesemery Comments addressed, back to you.

Copy link
Collaborator

@jamesemery jamesemery left a comment

Choose a reason for hiding this comment

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

Changes look good 👍

@droazen droazen merged commit 18e4fc5 into master Mar 27, 2019
@droazen droazen deleted the dr_PrintBGZFBlockInformation_allow_bams branch March 27, 2019 21:11
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