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.

Sign up for GitHub

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

Jump to bottom

ExtractCohort supports -XL exclusion and follows intervals, other optimizations #7181

Merged
merged 17 commits into from
Apr 6, 2021
Merged

ExtractCohort supports -XL exclusion and follows intervals, other optimizations #7181

merged 17 commits into from
Apr 6, 2021

Conversation

mmorgantaylor
Copy link
Member

@mmorgantaylor mmorgantaylor commented Apr 2, 2021 •
edited
Loading

  • added custom classes ExtractCohortRecord and ExtractCohortFilterRecord that implement Locatable
  • refactored attribute building from these records
  • now that the records are Locatables, can use OverlapDetector to filter locations down to only desired intervals (including excluded sites)
  • removed queryMode QUERY and associated querying from options

@mmorgantaylor mmorgantaylor requested review from kcibul and ahaessly April 2, 2021 19:11
mmorgantaylor
mmorgantaylor commented Apr 5, 2021
View reviewed changes
...org/broadinstitute/hellbender/tools/variantdb/nextgen/ExtractCohortFilterRecordUnitTest.java Outdated

@Test
public void testGetContig() {
String expectedContig = "chr1";
Copy link
Member Author

Choose a reason for hiding this comment

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

is it preferred to define these single-use objects explicitly or just stick "chr1" into the assertion statement directly?

Copy link
Contributor

Choose a reason for hiding this comment

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

either way is fine. I tend to put them right in the assertion so it's easier to read if I'm not also using it to construct the test data. In that case I pull it out so I don't have "chr1" repeated throughout the method

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah reading the rest of these tests I would just inline it into the assert for readability

kcibul
kcibul approved these changes Apr 5, 2021
View reviewed changes
Copy link
Contributor

@kcibul kcibul left a comment

Choose a reason for hiding this comment

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

Great refactoring

src/main/java/org/broadinstitute/hellbender/tools/variantdb/nextgen/ExtractCohort.java Outdated Show resolved Hide resolved
src/main/java/org/broadinstitute/hellbender/tools/variantdb/nextgen/ExtractCohortEngine.java
final Allele alt = Allele.create(queryRow.get("alt").toString(), false);
final ExtractCohortFilterRecord filterRow = new ExtractCohortFilterRecord( queryRow );

final long location = filterRow.getLocation();
Copy link
Contributor

Choose a reason for hiding this comment

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

So lovely!

src/main/java/org/broadinstitute/hellbender/tools/variantdb/nextgen/ExtractCohortEngine.java Outdated Show resolved Hide resolved
src/main/java/org/broadinstitute/hellbender/tools/variantdb/nextgen/ExtractCohortEngine.java
sampleRecords.add(new QueryRecord(row));

// TODO remove queryMode entirely from ExtractTool
throw new NotImplementedException("QUERY mode not supported. Please use `--query-mode LOCAL_SORT`.");
Copy link
Contributor

Choose a reason for hiding this comment

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

Yassssss!

src/main/java/org/broadinstitute/hellbender/tools/variantdb/nextgen/ExtractCohortEngine.java Outdated Show resolved Hide resolved
...org/broadinstitute/hellbender/tools/variantdb/nextgen/ExtractCohortFilterRecordUnitTest.java Outdated Show resolved Hide resolved
...org/broadinstitute/hellbender/tools/variantdb/nextgen/ExtractCohortFilterRecordUnitTest.java Outdated

@Test
public void testGetContig() {
String expectedContig = "chr1";
Copy link
Contributor

Choose a reason for hiding this comment

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

either way is fine. I tend to put them right in the assertion so it's easier to read if I'm not also using it to construct the test data. In that case I pull it out so I don't have "chr1" repeated throughout the method

...org/broadinstitute/hellbender/tools/variantdb/nextgen/ExtractCohortFilterRecordUnitTest.java Outdated

@Test
public void testGetContig() {
String expectedContig = "chr1";
Copy link
Contributor

Choose a reason for hiding this comment

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

yeah reading the rest of these tests I would just inline it into the assert for readability

.../java/org/broadinstitute/hellbender/tools/variantdb/nextgen/ExtractCohortRecordUnitTest.java Outdated Show resolved Hide resolved
.../java/org/broadinstitute/hellbender/tools/variantdb/nextgen/ExtractCohortRecordUnitTest.java Outdated Show resolved Hide resolved
@mmorgantaylor mmorgantaylor force-pushed the mmt_extract_cohort_records branch from 5b065b1 to 22fe7fc Compare April 5, 2021 20:55
@gatk-bot
Copy link

gatk-bot commented Apr 5, 2021 •
edited
Loading

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

Test Type JDK Job ID Logs
cloud openjdk8 33556.1 logs
cloud openjdk11 33556.14 logs

@mmorgantaylor mmorgantaylor force-pushed the mmt_extract_cohort_records branch from 22fe7fc to fc31d94 Compare April 6, 2021 17:21
@mmorgantaylor mmorgantaylor merged commit 2cbfc3a into ah_var_store Apr 6, 2021
@mmorgantaylor mmorgantaylor deleted the mmt_extract_cohort_records branch April 6, 2021 19:04
This was referenced Mar 17, 2023
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kcibul kcibul kcibul approved these changes

@ahaessly ahaessly Awaiting requested review from ahaessly

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mmorgantaylor @gatk-bot @kcibul