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

Innocuous housekeeping changes in the partially determined haplotypes code #8361

Merged
merged 33 commits into from
Jun 25, 2023

Conversation

davidbenjamin
Copy link
Contributor

@davidbenjamin davidbenjamin commented Jun 12, 2023

@jamesemery Getting into the PartiallyDeterminedHaplotypeComputationEngine now. No change in output here. I don't think there's anything controversial here. The commit history is discrete and pristine.

My next DRAGEN PR might be where the fun begins.

@gatk-bot
Copy link

gatk-bot commented Jun 13, 2023

Github actions tests reported job failures from actions build 5250024693
Failures in the following jobs:

Test Type JDK Job ID Logs
unit 17.0.6+10 5250024693.12 logs
unit 17.0.6+10 5250024693.1 logs
variantcalling 17.0.6+10 5250024693.2 logs

@gatk-bot
Copy link

Github actions tests reported job failures from actions build 5250310142
Failures in the following jobs:

Test Type JDK Job ID Logs
variantcalling 17.0.6+10 5250310142.2 logs

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.

Good refactor overall... almost all minor comments with one request for a unit test for a method that should have had one to begin with (hopefully not to onerous for you)

@@ -939,4 +939,9 @@ public AssemblyResultSet generateEmptyLLocalAssemblyResult(final AssemblyRegion

return resultSet;
}

private static String dotFileName(final Haplotype refHaplotype, final int kmerSize, final String suffix) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this can probably know that the suffix is ".dot"... also there is some logic that could go into this for the ordering of the steps (0.4->1.0->1.4 etc...) i suppose its too much to ask that the order of steps be managed by some tool...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wrote said tool. Was it overkill?

@davidbenjamin
Copy link
Contributor Author

Okay @jamesemery, ready for the next round of review.

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.

One very light request that you can check on your own. Otherwise these responses look good to me. 👍

@@ -214,4 +215,42 @@ public void testDeletionUnderlapingDeterminedBases() {
Assert.assertEquals(result.getCigar(), TextCigarCodec.decode("10M"));
Assert.assertEquals(result.getDeterminedPosition(), DEL_AA_105.getStart());
}

@Test
public void testEventsOverlapForPDHapsCode() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍 i think we are going to appreciate having this unit test in some vague undetermined point in the future

@davidbenjamin davidbenjamin merged commit a86d8a3 into master Jun 25, 2023
@davidbenjamin davidbenjamin deleted the db_pd_hap_cleaning branch June 25, 2023 04:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants