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

fix tabix index generation #7858

Merged
merged 2 commits into from Jun 7, 2022
Merged

fix tabix index generation #7858

merged 2 commits into from Jun 7, 2022

Conversation

tedsharpe
Copy link
Contributor

No description provided.

@tedsharpe tedsharpe requested a review from mwalker174 May 18, 2022 18:09
@tedsharpe
Copy link
Contributor Author

crude, but effective

@codecov
Copy link

codecov bot commented May 18, 2022

Codecov Report

Merging #7858 (8d1db54) into master (72684d0) will decrease coverage by 0.013%.
The diff coverage is 75.735%.

@@               Coverage Diff               @@
##              master     #7858       +/-   ##
===============================================
- Coverage     86.948%   86.936%   -0.013%     
- Complexity     36927     36945       +18     
===============================================
  Files           2219      2221        +2     
  Lines         173673    173803      +130     
  Branches       18755     18775       +20     
===============================================
+ Hits          151006    151097       +91     
- Misses         16055     16077       +22     
- Partials        6612      6629       +17     
Impacted Files Coverage Δ
...roadinstitute/hellbender/tools/DumpTabixIndex.java 70.536% <70.536%> (ø)
...itute/hellbender/utils/io/FeatureOutputStream.java 85.366% <100.000%> (+2.033%) ⬆️
...ellbender/tools/DumpTabixIndexIntegrationTest.java 100.000% <100.000%> (ø)
...llbender/utils/io/FeatureOutputStreamUnitTest.java 84.906% <100.000%> (+1.232%) ⬆️
.../hellbender/utils/python/PythonUnitTestRunner.java 75.410% <0.000%> (-3.279%) ⬇️
...e/hellbender/tools/sv/cluster/SVClusterEngine.java 93.269% <0.000%> (-1.002%) ⬇️
...stitute/hellbender/tools/walkers/sv/SVCluster.java 89.773% <0.000%> (-0.881%) ⬇️
...tools/walkers/sv/JointGermlineCNVSegmentation.java 86.047% <0.000%> (-0.752%) ⬇️
...der/tools/walkers/sv/SVClusterIntegrationTest.java 99.496% <0.000%> (-0.004%) ⬇️
...rs/haplotypecaller/graphs/AdaptiveChainPruner.java 97.368% <0.000%> (+0.035%) ⬆️
... and 3 more

Copy link
Contributor

@mwalker174 mwalker174 left a comment

Choose a reason for hiding this comment

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

Nice work here. I think the index dump tool could be quite useful / educational for some users, but it will need at least an integration test.

Also, a regression test for the bug fix would be ideal.

@droazen any comments on engine class changes or the tool?

@@ -47,11 +50,15 @@ public FeatureOutputStream( final GATKPath file,
Utils.nonNull(dict);
this.encoder = encoder;
if (IOUtil.hasBlockCompressedExtension(file.toPath())) {
outputStream = new PositionalOutputStream(
new BlockCompressedOutputStream(file.toString(), compressionLevel));
final BlockCompressedOutputStream bcos =
Copy link
Contributor

Choose a reason for hiding this comment

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

For posterity, can you add a comment explaining the necessity for BlockCompressedOutputStream instead of PositionalOutputStream?

final char meta = (char)readInt(is);
final int skip = readInt(is);
final int namesLen = readInt(is);
System.out.println("#tigs\tformat\tseqCol\tbegCol\tendCol\tmetaChr\tskip");
Copy link
Contributor

Choose a reason for hiding this comment

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

Preferable to have an output file argument instead of using stdout


@Override
protected Object doWork() {
try ( final InputStream is = new GZIPInputStream(new FileInputStream(tabixIndexFile)) ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a check that file has extension FileExtensions.TABIX_INDEX

import java.util.List;
import java.util.zip.GZIPInputStream;

@CommandLineProgramProperties(
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a docstring with explanation and usage, and annotate as a Barclay @DocumentedFeature

@Argument( doc = "Tabix index file.",
fullName = "tabix-index",
shortName = StandardArgumentDefinitions.INPUT_SHORT_NAME)
String tabixIndexFile;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
String tabixIndexFile;
GATKPath tabixIndexFile;

@droazen
Copy link
Collaborator

droazen commented Jun 6, 2022

@tedsharpe Any chance this might resolve the issue reported in #7838, or is that likely unrelated?

@tedsharpe
Copy link
Contributor Author

@droazen I believe it's unrelated.

@tedsharpe
Copy link
Contributor Author

I believe the review comments are all addressed, and that this is ready to go. (Assuming checks all pass.)

@tedsharpe tedsharpe merged commit d4f083d into master Jun 7, 2022
@tedsharpe tedsharpe deleted the tws_fixTabixIndex branch June 7, 2022 19:02
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.

None yet

3 participants