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

fixed an error where the read mate key in SplitNCigarReads was not sufficiently strict about readnames causing cigar errors #6909

Merged
merged 5 commits into from Nov 16, 2020

Conversation

jamesemery
Copy link
Collaborator

Fixes #6776

@gatk-bot
Copy link

gatk-bot commented Oct 23, 2020

Travis reported job failures from build 31916
Failures in the following jobs:

Test Type JDK Job ID Logs
cloud openjdk11 31916.14 logs
cloud openjdk11 31916.14 logs
cloud openjdk8 31916.1 logs
cloud openjdk11 31916.14 logs
cloud openjdk8 31916.1 logs
cloud openjdk8 31916.1 logs
cloud openjdk11 31916.14 logs
cloud openjdk8 31916.1 logs
cloud openjdk8 31916.1 logs
cloud openjdk11 31916.14 logs

@gatk-bot
Copy link

Travis reported job failures from build 31925
Failures in the following jobs:

Test Type JDK Job ID Logs
cloud openjdk8 31925.1 logs

Copy link
Member

@lbergelson lbergelson left a comment

Choose a reason for hiding this comment

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

This looks fine to me. Test are broken for mysterious reasons right? What's with the sudos? Did you test what happens if someone has illegal read names? I'm just curious if we ban it or it just silently goes through and breaks stuff.

.travis.yml Outdated
@@ -110,14 +110,14 @@ install:
elif [[ $TEST_DOCKER == true ]]; then
echo "Skip the install because we're doing a docker build";
else
./gradlew assemble;
./gradlew installDist;
sudo ./gradlew assemble;
Copy link
Member

Choose a reason for hiding this comment

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

why?

@VisibleForTesting
static String makeKey(String name, boolean firstOfPair, int mateStart) {
// NOTE: We use the '@' character here because it is explicitly forbidden for use in read names by the SAM Spec and we don't want to risk accidental matches across read pairs
return name + "@" + (firstOfPair ? 1 : 0) + "@" + mateStart;
Copy link
Member

Choose a reason for hiding this comment

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

👍

@@ -201,6 +201,20 @@ public void testMappingReadMateRepair() {
Assert.assertEquals(read2.getMateStart(), 10003);
}

@Test
Copy link
Member

Choose a reason for hiding this comment

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

@jamesemery Have you tested what happens if you add a read with a @s in the name which could recreate the problem?

Copy link
Member

@lbergelson lbergelson left a comment

Choose a reason for hiding this comment

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

Merge when tests are passing.

@lbergelson lbergelson assigned jamesemery and unassigned lbergelson Oct 29, 2020
@gatk-bot
Copy link

Travis reported job failures from build 31969
Failures in the following jobs:

Test Type JDK Job ID Logs
cloud openjdk8 31969.1 logs

@gatk-bot
Copy link

Travis reported job failures from build 31971
Failures in the following jobs:

Test Type JDK Job ID Logs
cloud openjdk8 31971.1 logs

@gatk-bot
Copy link

gatk-bot commented Oct 29, 2020

Travis reported job failures from build 31973
Failures in the following jobs:

Test Type JDK Job ID Logs
cloud openjdk8 31973.1 logs
cloud openjdk8 31973.1 logs
cloud openjdk8 31973.1 logs
conda openjdk8 31973.5 logs

@gatk-bot
Copy link

gatk-bot commented Nov 9, 2020

Travis reported job failures from build 32069
Failures in the following jobs:

Test Type JDK Job ID Logs
cloud openjdk8 32069.1 logs
cloud openjdk8 32069.1 logs
conda openjdk8 32069.5 logs
conda openjdk8 32069.5 logs
conda openjdk8 32069.5 logs
conda openjdk8 32069.5 logs
conda openjdk8 32069.5 logs
conda openjdk8 32069.5 logs

@jamesemery jamesemery merged commit 2498001 into master Nov 16, 2020
@jamesemery jamesemery deleted the je_fixSplitNCigarReadsCrash branch November 16, 2020 17:51
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.

Error in SplitNCigarReads v4.8.1.0
3 participants