minor fixes 0525 #133
minor fixes 0525 #133
Conversation
| @@ -911,8 +911,8 @@ def is_sample_sheet(sample_sheet_path): | |||
|
|
|||
| return False | |||
|
|
|||
| def _generate_dummy_sample_sheet(self, first_read, last_read, | |||
| indexed_reads, dummy_sample_id): | |||
| def _generate_dummy_sample_sheet(self, index_cycles, non_index_cycles, | |||
There was a problem hiding this comment.
FWIW, the code expected (as this is how old RunInfo.xml files are formed) that the reads would be like index1, read1, read2, index2 - however this is not the case in the new versions. Thus, changed the method to actually be aware of what's an index and non_index read
There was a problem hiding this comment.
I am 100% in favor of anything that makes the nature of the inputs more explicit! However, I don't really understand what the inputs should be here. Is it possible to add a docstring for this function, maybe with an example input for each param?
| @@ -19,6 +19,7 @@ class WorkflowFactory(): | |||
| ST_TO_IN_MAP = {PROTOCOL_NAME_ILLUMINA: ['standard_metag', | |||
| 'standard_metat', | |||
| 'absquant_metag', | |||
| 'abs_quant_metag', | |||
There was a problem hiding this comment.
Given that this module already depends on metapool, I would really, really like it if, instead of redefining strings for the same sheet-type names that are (canonically) defined in metapool.sample_sheet, we just used the public values defined there:
Also, there still is no absquant_metat (and there are no plans that there ever will be). Using (only) the sheet types from in the module where we actually define the sheet types would ensure that we never have this sort of nonsense entry in the future.
| @@ -28,7 +28,7 @@ | |||
| class InstrumentUtils(): | |||
| types = {'A': 'NovaSeq 6000', 'D': 'HiSeq 2500', 'FS': 'iSeq', | |||
| 'K': 'HiSeq 4000', 'LH': 'NovaSeq X Plus', 'M': 'MiSeq', | |||
| 'MN': 'MiniSeq', | |||
| 'MN': 'MiniSeq', 'SH': 'MiSeq i100', | |||
There was a problem hiding this comment.
I am worried about this one, not for this code, but for kl-metapool, which AFAICT doesn't know about MiSeq i100 (as opposed to just regular MiSeq). What do we know about whether it needs its i5 barcode revcomped or not, etc? Is kl-metapool going to need modifications to handle this sequencer?
| @@ -911,8 +911,8 @@ def is_sample_sheet(sample_sheet_path): | |||
|
|
|||
| return False | |||
|
|
|||
| def _generate_dummy_sample_sheet(self, first_read, last_read, | |||
| indexed_reads, dummy_sample_id): | |||
| def _generate_dummy_sample_sheet(self, index_cycles, non_index_cycles, | |||
There was a problem hiding this comment.
I am 100% in favor of anything that makes the nature of the inputs more explicit! However, I don't really understand what the inputs should be here. Is it possible to add a docstring for this function, maybe with an example input for each param?
| if (first_read['IsIndexedRead'] is True or | ||
| last_read['IsIndexedRead'] is True): | ||
| # the assumptions are: if we have 3 reads we should only have 1 | ||
| # index; and if we have 4 reads, 2 should be index |
| @@ -0,0 +1,26 @@ | |||
| <?xml version="1.0"?> | |||
There was a problem hiding this comment.
Is this one from a NovaSeqXPlus?
|
Really good points. I added this PR biocore/kl-metapool#301 so we can cover 2 of your comments but they need to be merged there before I can use them here. |
No description provided.