Allow explicit index file paths for inputs to CrosscheckFingerprints #1823

rickymagner · 2022-08-16T01:03:21Z

Description

This PR addresses an issue regarding data storage solutions where indexed files can be separated from their indices. CrosscheckFingerprints will now allow for an INPUT_INDEX_MAP tsv file with two columns: the first being the list of input files, and the latter being the corresponding index file locations. One can also do the same for the SECOND_INPUT arguments using SECOND_INPUT_INDEX_MAP.

Some tests have been added to test this functionality. This involved copying some test files into a separate dir from index files which were generated in a different directory. The test generates the INPUT_INDEX_MAP file on the fly so as to not rely on any absolute file paths.

Although the tool still runs if an index is not provided, a new warning will be written if providing an index map without mapping each input to an index. If an index file path is typed incorrectly in the map, an error should be thrown by some dependency classes about not being able to import a valid index from the given path.

Checklist

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

…inputs; added some tests for this new functionality.

rickymagner · 2022-08-16T01:07:26Z

Here is the update to CrosscheckFingerprints as promised. I would appreciate a review from anyone who has time to take a look, especially any number of: @droazen, @yfarjoun, @meganshand, @kachulis, @lbergelson. Thanks!

meganshand

Thanks @rickymagner!

src/main/java/picard/fingerprint/CrosscheckFingerprints.java

src/main/java/picard/fingerprint/FingerprintChecker.java

src/test/java/picard/fingerprint/CrosscheckFingerprintsTest.java

…test.

rickymagner · 2022-08-17T15:09:18Z

Added another commit to allow for an INPUT_FORCE_INDEX argument. When toggled on, this checks that all files read for fingerprinting are using indices, either from those provided in INPUT_INDEX_MAP or found by the usual methods. The corresponding test for this PR has been updated to toggle this on (as it's off by default). The SECOND_INPUTs have the same option added.

I'm working on one more test to make sure when this option is on and inputs don't have a valid index, then the right exception should be thrown.

rickymagner · 2022-08-17T15:13:47Z

In the meantime, I refactored the SAM file parsing a bit at this line. Previously this had a hot fix to address this issue.

It would be nice if someone with a bit more knowledge about SamInputResource could comment as to whether my code would fall into the same problem, or if it's avoided by using this different htsjdk code. Maybe one of @yfarjoun / @lbergelson / @cmnbroad if they have a moment? Thanks

src/main/java/picard/fingerprint/CrosscheckFingerprints.java

src/main/java/picard/fingerprint/FingerprintChecker.java

src/test/java/picard/fingerprint/CrosscheckFingerprintsTest.java

src/main/java/picard/fingerprint/FingerprintChecker.java

rickymagner · 2022-08-17T20:16:34Z

Added the last test as promised above for the INPUT_INDEX_FORCE option, and addressed the comments above. The only thing I'm waiting on is a bit more review + someone potentially addressing this concern.

meganshand

This looks much cleaner and thank you for adding the additional tests!

src/test/java/picard/fingerprint/CrosscheckFingerprintsTest.java

src/main/java/picard/fingerprint/CrosscheckFingerprints.java

src/main/java/picard/fingerprint/FingerprintChecker.java

src/main/java/picard/fingerprint/CrosscheckFingerprints.java

lbergelson · 2022-08-23T19:32:27Z

src/main/java/picard/fingerprint/FingerprintChecker.java

+    private SamReader getSamReader(final Path samFile, final Path indexPath, final boolean forceIndex) {
+        final SamInputResource samResource = SamInputResource.of(samFile);
+        if (indexPath != null) {
+            samResource.index(indexPath);


Suggested change

samResource.index(indexPath);

samResource.index(indexPath, seekableChannelFunction);

Thanks, just implemented this change and checked this should be equivalent to the old code. I left a comment pointing to the relevant issue in case someone tries to delete the seekableChannelFunction wrapper.

…reaming index.

kachulis

Just a couple of small things, other wise looks good. Thanks @rickymagner!

kachulis · 2022-08-25T15:21:17Z

src/main/java/picard/fingerprint/FingerprintChecker.java

@@ -672,27 +684,19 @@ public Map<FingerprintIdDetails, Fingerprint> fingerprintFiles(final Collection<
                final Map<FingerprintIdDetails, Fingerprint> oneFileFingerprints;
                log.debug("Processed file: " + p.toUri().toString() + " (" + filesRead.get() + ")");

+                // Determine whether valid index path given for this file
+                final Path indexPath = (indexPathMap != null && indexPathMap.containsKey(p)) ? indexPathMap.get(p) : null;


Don't need to check that p is in indexPathMap. indexPathMap.get(p) will return null if it can't find p in the map

Nice, removed the second boolean expression for the ternary check

src/test/java/picard/fingerprint/CrosscheckFingerprintsTest.java

src/main/java/picard/fingerprint/CrosscheckFingerprints.java

src/main/java/picard/fingerprint/FingerprintChecker.java

kachulis · 2022-08-25T15:44:04Z

src/main/java/picard/fingerprint/FingerprintChecker.java

-    public Map<String, Fingerprint> loadFingerprints(final Path fingerprintFile, final String specificSample) {
-        final VCFFileReader reader = new VCFFileReader(fingerprintFile, false);
+    public Map<String, Fingerprint> loadFingerprints(final Path fingerprintFile, final Path indexPath, final boolean forceIndex, final String specificSample) {
+        final VCFFileReader reader = getVCFReader(fingerprintFile, indexPath, forceIndex);


what about

final VCFFileReader reader = new VCFFileReader(fingerprintFile, indexPath, forceIndex)

it looks like that constructor should handle a null indexPath correctly.

This actually causes a bunch of tests to fail (apparently due to some null pointer exception). Although it seems like the method accepts null pointers, it looks like htsjdk handles that by assuming the index is next to the input file with extension .tbi which many of our test files don't have. I think it might be safer to leave it as is for now. This also allows for the custom PicardException to display when the user forces index file paths but they cannot be found.

src/test/java/picard/fingerprint/CrosscheckFingerprintsTest.java

…tions.

kachulis · 2022-08-26T15:37:26Z

src/main/java/picard/fingerprint/FingerprintChecker.java

+        final SamInputResource samResource = SamInputResource.of(samFile);
+        if (indexPath != null) {
+            // Use of seekableChannelFunction here avoids issue: https://github.com/broadinstitute/picard/issues/1175
+            samResource.index(indexPath, seekableChannelFunction);


talked about this offline, but it looks like when using a SamInputResource, you need to explicitly give it an index if you want one to be used. So you need to find the index if you haven't been given it explicitly. See, as an example, the SamReader open method which takes a Path. Would also be good to add a test to confirm that the index is used when it is available if not explicitly given as input.

Good catch! Fixed and also added a test that should address this.

…g index when nearby but not provided.

lbergelson

@rickymagner Sorry for the delay. I have some comments. The ones about the UI you can ignore if you feel like this is the cleanest way, but I think having a single input that can have a 3rd column with matched indexes like what David added recently in GATK (and we can maybe steal the loading code for) would be clearer.

I also am not sure I understand why you need 2 force index arguments, it seems extremely error prone. I will 100% forget to force the second set of indexes every time.

src/main/java/picard/cmdline/CommandLineProgram.java

src/main/java/picard/fingerprint/CrosscheckFingerprints.java

src/main/java/picard/fingerprint/FingerprintChecker.java

src/test/java/picard/fingerprint/CrosscheckFingerprintsTest.java

lbergelson · 2022-09-06T18:46:38Z

@rickymagner Thanks for addressing those. Some comments are more aspirational than practical. Looks good to me.

lbergelson

👍

src/main/java/picard/fingerprint/CrosscheckFingerprints.java

src/main/java/picard/cmdline/CommandLineProgram.java

src/main/java/picard/fingerprint/CrosscheckFingerprints.java

kachulis

Looks good, thanks @rickymagner!

rickymagner and others added 5 commits August 2, 2022 23:19

Fixed type in CommandLineProgram comment.

2b0c50f

Modified CrosscheckFingerprints to allow for explicit index paths w/ …

0d254c9

…inputs; added some tests for this new functionality.

Updated documentation formatting for CrosscheckFingerprints.

d91f944

Removed debugging print lines.

2f02554

Removed aux file used in earlier test version.

a34cb39

rickymagner requested review from meganshand and droazen August 16, 2022 01:07

meganshand reviewed Aug 16, 2022

View reviewed changes

Added option to force use of indices; refactored code a bit; updated …

a89ea6f

…test.

kachulis requested changes Aug 17, 2022

View reviewed changes

Addressed comments and added test for INPUT_INDEX_FORCE option

f0ad017

Made variables final in getPathPathMap

559bd06

meganshand approved these changes Aug 18, 2022

View reviewed changes

src/test/java/picard/fingerprint/CrosscheckFingerprintsTest.java Show resolved Hide resolved

src/main/java/picard/fingerprint/CrosscheckFingerprints.java Outdated Show resolved Hide resolved

Clarified use case for INPUT_INDEX_FORCE option.

ba584c3

kachulis reviewed Aug 18, 2022

View reviewed changes

src/main/java/picard/fingerprint/FingerprintChecker.java Outdated Show resolved Hide resolved

src/main/java/picard/fingerprint/FingerprintChecker.java Outdated Show resolved Hide resolved

src/main/java/picard/fingerprint/CrosscheckFingerprints.java Outdated Show resolved Hide resolved

Small edits and clarified FORCE_INDEX options may throw exception.

b3b7029

lbergelson reviewed Aug 23, 2022

View reviewed changes

rickymagner added 2 commits August 23, 2022 21:34

Fixed Sam file parsing to avoid reintroducing old issue with cloud st…

69dc558

…reaming index.

Add comment about issue related to old Sam file parsing.

95ee1e1

kachulis approved these changes Aug 25, 2022

View reviewed changes

Refactored test code a bit, fixed docs typos, and added final declara…

ec8d26d

…tions.

kachulis requested changes Aug 26, 2022

View reviewed changes

Allow SAM files to seach for nearby indices; added test to check usin…

8c02218

…g index when nearby but not provided.

lbergelson requested changes Sep 1, 2022

View reviewed changes

lbergelson reviewed Sep 1, 2022

View reviewed changes

src/test/java/picard/fingerprint/CrosscheckFingerprintsTest.java Outdated Show resolved Hide resolved

Incorporated some recommended changes for readability and other fixes.

56af8e8

lbergelson approved these changes Sep 6, 2022

View reviewed changes

Changed 'field' name to 'argument' for better readability

129047b

kachulis approved these changes Sep 7, 2022

View reviewed changes

rickymagner merged commit fc87aed into master Sep 8, 2022

rickymagner deleted the rm_fingerprint_allow_index_paths branch September 8, 2022 14:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow explicit index file paths for inputs to CrosscheckFingerprints #1823

Allow explicit index file paths for inputs to CrosscheckFingerprints #1823

rickymagner commented Aug 16, 2022 •

edited

Loading

rickymagner commented Aug 16, 2022

meganshand left a comment

rickymagner commented Aug 17, 2022

rickymagner commented Aug 17, 2022

rickymagner commented Aug 17, 2022

meganshand left a comment

lbergelson Aug 23, 2022

rickymagner Aug 24, 2022

kachulis left a comment

kachulis Aug 25, 2022

rickymagner Aug 25, 2022

kachulis Aug 25, 2022

rickymagner Aug 25, 2022

kachulis Aug 26, 2022

rickymagner Aug 29, 2022

lbergelson left a comment

lbergelson commented Sep 6, 2022

lbergelson left a comment

kachulis left a comment

	samResource.index(indexPath);
	samResource.index(indexPath, seekableChannelFunction);

Allow explicit index file paths for inputs to CrosscheckFingerprints #1823

Allow explicit index file paths for inputs to CrosscheckFingerprints #1823

Conversation

rickymagner commented Aug 16, 2022 • edited Loading

Description

Checklist

Content

Review

rickymagner commented Aug 16, 2022

meganshand left a comment

Choose a reason for hiding this comment

rickymagner commented Aug 17, 2022

rickymagner commented Aug 17, 2022

rickymagner commented Aug 17, 2022

meganshand left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kachulis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbergelson left a comment

Choose a reason for hiding this comment

lbergelson commented Sep 6, 2022

lbergelson left a comment

Choose a reason for hiding this comment

kachulis left a comment

Choose a reason for hiding this comment

rickymagner commented Aug 16, 2022 •

edited

Loading