-
Notifications
You must be signed in to change notification settings - Fork 4
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
WIP: production hotfixes #98
base: main
Are you sure you want to change the base?
Conversation
# if successful, set self.reports_path | ||
self.reports_path = join(self.pipeline.output_path, | ||
'SeqCountsJob', | ||
'SeqCounts.csv') |
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.
Is this file consistent in schema with Demultiplex_Stats.csv
used in the other object for reports_path
?
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.
No Demultiplex_Stats.csv's schema is defined and maintained by Illumina and carries additional stats beyond sequence counts. SeqCounts.csv is defined and maintained by us and just records the total raw-read counts for forward and reverse and the lane number per sample.
Consistency in this case is not an issue since the consumer of the metadata is the metapool module, and it only uses Demultuplex_Stats.csv for its raw-read counts. The equivalent output file for the bcl2fastq is similarly different. Our new method is more accurate in that it counts both the forward and reverse reads while I believe in Demultuplex_Stats.csv we just double the forward count.
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.
Isn't it risky, and unexpected, for an attribute common across subclasses to have a different interpretation?
# shared method, but probably still in Assay. | ||
self.generate_prep_file() | ||
|
||
self.generate_prep_file() |
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.
It seems like the stuff in this method is the same as what's in the StandardMetagenomicWorkflow
, can execute_pipeline
be pushed to a base class?
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.
I could push much of both into the MetaOmics base class off the top of my head. Those two child classes are very similar. Currently they differ only in the string constants we define, off the top of my head. More than happy to include that in my next push.
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.
Seems like a good idea?
Production hotfixes. Tests not yet updated.