-
Notifications
You must be signed in to change notification settings - Fork 1
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
Functionality: Adding sequencing-type as a parameter to allow ingestion of exome data #666
Functionality: Adding sequencing-type as a parameter to allow ingestion of exome data #666
Conversation
…data comes along and sequencing groups can be ingested as exomes instead of defaulting to genome
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Take a look at your linting failures & test failures. In this case, you need to update the relevant tests in line with your new change. You can also add new tests to capture this new case.
scripts/parse_existing_cohort.py
Outdated
@click.option( | ||
'--sequencing-type', | ||
type=click.Choice(['genome', 'exome']), | ||
required=True, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to set required to be true? Esp since we have a default 'genome' described below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call!
… default set to genome
…s set correctly in the ExistingCohortParser class when the --sequencing-type flag is set to 'genome' or 'exome'. The test checks both 'genome' and 'exome' options.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev #666 +/- ##
==========================================
+ Coverage 68.57% 68.67% +0.09%
==========================================
Files 131 131
Lines 10166 10198 +32
==========================================
+ Hits 6971 7003 +32
Misses 3195 3195 ☔ View full report in Codecov by Sentry. |
test/test_parse_existing_cohort.py
Outdated
@@ -280,3 +285,113 @@ async def test_get_read_filenames_no_reads_pass(self): | |||
self.assertIn('No read files found for ', cm.output[0]) | |||
|
|||
self.assertEqual(len(read_filenames), 0) | |||
|
|||
@run_as_sync | |||
async def test_exome_sequencing_type(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe either rename or split into two so it's accurate to what we're testing
Adding sequencing-type as a parameter for future-proofing when exome data comes along and sequencing groups can be ingested as exomes instead of defaulting to genome.
At the moment, default_sequencing_type is set to genome, this fix would allow default_sequencing_type to be either exome or genome