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

Fixing a bug in parsing of T0 tag #8185

Conversation

ilyasoifer
Copy link
Collaborator

No description provided.

@ilyasoifer ilyasoifer force-pushed the ilyasoifer/bioin-882-fix.t0.parse.bug branch from 5a5cf18 to 1b1e601 Compare February 5, 2023 11:57
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.

Looks good and very simple but I scratch my head at understanding how this happened.

if ((probs[qualOfs-1])==(probs[qualOfs])){
flowMatrix[1][flowIdx] = Math.max(flowMatrix[1][flowIdx], Math.max(probs[qualOfs-1],probs[qualOfs]));
}
flowMatrix[1][flowIdx] = Math.max(flowMatrix[1][flowIdx], Math.min(probs[qualOfs-1],probs[qualOfs]));
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 provide a bit of context as to what the expectation is/was here?

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

// instead of actually running the tests. Can be used with "./gradlew test -Dtest.single=HaplotypeCallerIntegrationTest"
// to update all of the exact-match tests at once. After you do this, you should look at the
// diffs in the new expected outputs in git to confirm that they are consistent with expectations.
public static final boolean UPDATE_EXACT_MATCH_EXPECTED_OUTPUTS = false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

good to add.

322,0,0,.,.,.,.,. 0.9880
322,1,0,.,.,.,.,. 0.0010
322,0,0,.,.,.,.,. 0.8890
322,1,0,.,.,.,.,. 0.1000
Copy link
Collaborator

Choose a reason for hiding this comment

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

These seem like fairly small effects...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right. And from the beginning the effect is small. But the fix does seem to improve SNPs a tiny bit

@ilyasoifer ilyasoifer merged commit b4682ba into broadinstitute:master Feb 21, 2023
@ilyasoifer ilyasoifer deleted the ilyasoifer/bioin-882-fix.t0.parse.bug branch February 21, 2023 16:03
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.

2 participants