OT-2 protocol configured for running via SSH #22
Conversation
Here is the full version of the OT-2 protocol for library prep. A write-up for adapting the workflow around the OT-2 will be posted later on, but here is a summary of the scope of the script:
ot2/octopus-full-ssh.py
Outdated
| @@ -0,0 +1,590 @@ | |||
| import sys | |||
There was a problem hiding this comment.
Could you include a succinct module docstring here? The description for this PR is an excellent write-up and some of those bullet points would be great to include.
There was a problem hiding this comment.
Yes. I'll add descriptions for the other methods as well.
| if NUM_PLATES >= 1: | ||
| self.p300.consolidate(5, [self.pre_pool_plate_1.rows_by_name()['A'][num_plate].bottom(z=0.5) for num_plate in range(0, 23, 2)], self.consolidation_plate.columns('1')) | ||
| if NUM_PLATES >= 2: | ||
| self.p300.consolidate(5, [self.pre_pool_plate_1.rows_by_name()['A'][num_plate].bottom(z=0.5) for num_plate in range(1, 24, 2)], self.consolidation_plate.columns('2')) | ||
| if NUM_PLATES >= 3: | ||
| self.p300.consolidate(5, [self.pre_pool_plate_1.rows_by_name()['B'][num_plate].bottom(z=0.5) for num_plate in range(0, 23, 2)], self.consolidation_plate.columns('3')) | ||
| if NUM_PLATES >= 4: | ||
| self.p300.consolidate(5, [self.pre_pool_plate_1.rows_by_name()['B'][num_plate].bottom(z=0.5) for num_plate in range(1, 24, 2)], self.consolidation_plate.columns('4')) | ||
| # consolidating VE plates 5-8 to Pool Plate columns 5-8 | ||
| if NUM_PLATES >= 5: | ||
| self.p300.consolidate(5, [self.pre_pool_plate_2.rows_by_name()['A'][num_plate].bottom(z=0.5) for num_plate in range(0, 23, 2)], self.consolidation_plate.columns('5')) | ||
| if NUM_PLATES >= 6: | ||
| self.p300.consolidate(5, [self.pre_pool_plate_2.rows_by_name()['A'][num_plate].bottom(z=0.5) for num_plate in range(1, 24, 2)], self.consolidation_plate.columns('6')) | ||
| if NUM_PLATES >= 7: | ||
| self.p300.consolidate(5, [self.pre_pool_plate_2.rows_by_name()['B'][num_plate].bottom(z=0.5) for num_plate in range(0, 23, 2)], self.consolidation_plate.columns('7')) | ||
| if NUM_PLATES == 8: | ||
| self.p300.consolidate(5, [self.pre_pool_plate_2.rows_by_name()['B'][num_plate].bottom(z=0.5) for num_plate in range(1, 24, 2)], self.consolidation_plate.columns('8')) |
There was a problem hiding this comment.
I'm getting a bad smell here. I realize it might be more explicit just to write all these commands out, but is there a reason why this wouldn't work?
for plate_num in range(1, NUM_PLATES + 1):
pre_pool_plate = self.pre_pool_plate_1 if plate_num > 4 else self.pre_pool_plate2
if plate_num % 2 == 1:
wells = [self.pre_pool_plate1.rows_by_name()['B'][row_n].bottom(z=0.5) for row_n in range(0, 23, 2)]
else:
wells = [self.pre_pool_plate1.rows_by_name()['A'][row_n].bottom(z=0.5) for row_n in range(1, 24, 2)]
self.p300.consolidate(volume=5, source=wells, dest=self.consolidation_plate.columns(str(plate_num))There was a problem hiding this comment.
Yeah... I was going back and forth on how that part should be written but ended up keeping it more explicit. The reason was for user flexibility.
For example, there might be a case where the runner only has 2 separate pre-stocked 384-well reagent plates pre_pool_plate_1 pre_pool_plate_2 prepped for, say, 3-plate and 2-plate runs respectively. When a 5-plate run is required, the runner could use those two pre-stocked plates and then just edit the protocol to reflect which columns to consolidate from.
The protocol by default will assume pre_pool_plate_1 to contain 4 plates and pre_pool_plate_2 to contain 1 plate. Having the code more explicit makes it arguably more straightforward to adapt the protocol to such a scenario as to avoid having to take the time pre-stocking reagent plates fresh.
Having something like what you proposed would definitely make this block more neat and tidy. However, the rows and columns alternate in separate patterns (not just singly even-odd for both). That is, the rows rows_by_name()['A'] and rows_by_name()['B'] alternate every two consolidation steps while the columns in range(0, 23, 2) and range(1, 24, 2) alternate every single consolidation step. I just felt that generalizing this consolidation pattern for the application where the maximum consolidation steps is only 8 would be unnecessarily restricting.
There was a problem hiding this comment.
This is awesome -- very thorough! In order to improve its maintainability, it would be nice to split up the large (multi-hundred line) functions into smaller functions and to factor out atomic actions (e.g. wash with ethanol) into their own functions. I've commented a few examples of where I think this makes sense, but didn't go through everything -- let me know if you want to chat more in depth about each example
| ### add and incubate Purification Beads with pooled samples | ||
| self.p300.pick_up_tip() | ||
| self.set_flow_rate(48*4, 48*4, 48) | ||
| self.p300.mix(10, 200, Purification_Beads.bottom(z=10.0)) | ||
| # adding beads to pool | ||
| self.set_flow_rate(48*2, 48*4, 48) | ||
| for i in range(3): | ||
| self.p300.aspirate(200, Purification_Beads) | ||
| self.p300.dispense(200, DNA_pool) | ||
| self.p300.aspirate(120, Purification_Beads) | ||
| self.p300.dispense(120, DNA_pool) | ||
| # mix beads well with pool and incubate | ||
| self.set_flow_rate(48*4, 48*6, 48) | ||
| self.p300.mix(10, 200, DNA_pool.bottom(z=10.0)) | ||
| self.set_flow_rate(48*4, 48*4, 48) | ||
| for i in range(20): | ||
| self.p300.aspirate(200, DNA_pool.bottom(z=10.0)) | ||
| self.p300.dispense(200, DNA_pool.center()) | ||
| self.set_flow_rate(48*4, 48*6, 48/6) | ||
| self.p300.mix(20, 200, DNA_pool.bottom(z=10.0)) | ||
| self.p300.move_to(DNA_pool.top()) | ||
| self.protocol.delay(seconds=5.0) | ||
| self.p300.blow_out(DNA_pool.top()) | ||
| self.p300.drop_tip() | ||
| self.protocol.delay(minutes=10) | ||
| self.mag_mod.engage() | ||
| self.protocol.delay(minutes=12) |
There was a problem hiding this comment.
Let's separate each of these sections into a separate function (it's easier to read)
There was a problem hiding this comment.
This is generally true throughout this file
There was a problem hiding this comment.
Yes, I understand where you are coming from. Many sections throughout the protocol indeed have repeated units of code.
However, many have intricate differences, such as differences in delay timing and pipette z positioning. While these details technically could be passed as arguments within methods, I feel that readability might be more compromised that way with having a wall of out-of-context parameters as opposed to how it is structured currently where I attempt to alleviate the problem by adding line breaks and comments between meaningful operations. This is arguably especially true since these operations are highly localized and don't occur outside of those comment blocks.
Operations not locally unique I have created methods for, such as set_flow_rate and discard_supernatant.
There was a problem hiding this comment.
That all sounds a bit hypocritical given that the whole protocol itself is contained within two giant functions Pool() and Post_Pool(), but I believe having these super high-level sections be separated using massive functions contributes more to readability than having the highly localized low-level sections be their own functions.
There was a problem hiding this comment.
Hmmmm I'm not sure I agree here. If I'm a new developer to this file, I would much rather look at the Pool() function and see:
incubate_purification_beads()
discard_supernatant()
ethanol_wash()
resuspend_beads()
...
etc. than a 500 line function. That way I can drop into just the function that is relevant to what I'm looking at
| self.p300.aspirate(200, DNA_pool.bottom(z=0.5)) | ||
| self.p300.dispense(200, Purification_Beads_discard) | ||
| self.p300.move_to(Purification_Beads_discard.top(z=10.0)) | ||
| self.protocol.delay(minutes=2) |
There was a problem hiding this comment.
Mmmm yes, it was uncomfortable leaving this section rolled out like this, and I would have it wrapped up in a loop-like structure if I had to otherwise. However, I thought having a method like
pool_stage_bead_supernant_discard(volume, source, destination, delay, has_delay=True, bottom=0.5, top=10.0)
in such a localized area seemed less readable that just having the area isolated by comment line breaks.
There was a problem hiding this comment.
Hmmm I think that function is much more readable than 40 lines of very similar looking text. You can remove the has_delay argument and just set delay to -1 if you want no delay if you wanted to make it a little cleaner
There was a problem hiding this comment.
delays = [4, 2, 2, 2, 2, None]
for delay in delays:
self.p300.aspirate(200, DNA_pool.bottom(z=0.5))
self.p300.dispense(200, Purification_Beads_discard)
self.p300.move_to(Purification_Beads_discard.top(z=10.0))
if delay:
self.protocol.delay(mintues=delay)| self.set_flow_rate(48, 48, 48) | ||
| self.p300.pick_up_tip() | ||
| for i in range(6): | ||
| self.p300.aspirate(200, Ethanol[1]) | ||
| self.p300.dispense(200, DNA_pool.top(z=-10.0)) | ||
| self.p300.move_to(DNA_pool.top(z=10.0)) | ||
| self.protocol.delay(seconds=40) | ||
| self.discard_supernatant(1200, DNA_pool, Ethanol_discard[1].top(z=-10.0)) |
There was a problem hiding this comment.
Do you think having a situation where a procedural unit like this repeated only twice and existing side-by-side only in this area of the whole protocol merits its own function? Would it be that much more readable than how it currently is, line-break separated with comments?
There was a problem hiding this comment.
I actually think this shouldn't be a function, but should be a for loop over zip(Ethanol, Ethanol_discard). I think that's much more readable than trying to understand what the differences between the times are.
| self.set_flow_rate(5, 48, 48) | ||
| self.p20.pick_up_tip() | ||
| self.p20.aspirate(20, DNA_pool.bottom(z=-0.5)) | ||
| self.p20.drop_tip() |
There was a problem hiding this comment.
These chunks are modified versions of the discard_residual method but applied to the context of this specific part of the Pool stage. However, this modified discard_residual is used nowhere else and differs I think significantly enough from the general discard_residual that I don't think merits its own function.
There was a problem hiding this comment.
self.discard_ethanol(z=-0.5, delay=10)
self.discard_ethanol(z=-0.5, delay=10)
self.discard_ethanol(z=-1, delay=5)
There was a problem hiding this comment.
I think we have a fundamental disagreement about readability going on here. In my view, it's much more readable for a developer to see a few well named function calls and then be able to click into them if they need more granular details. I will say that even just approaching this PR was difficult for me, since I had to read every single instruction that the OT2 will execute.
I'll approve it and you can merge if you'd like, but I think this will be hard for any new developer who's not in the lab actually executing all of these steps to reason about.
| ### add and incubate Purification Beads with pooled samples | ||
| self.p300.pick_up_tip() | ||
| self.set_flow_rate(48*4, 48*4, 48) | ||
| self.p300.mix(10, 200, Purification_Beads.bottom(z=10.0)) | ||
| # adding beads to pool | ||
| self.set_flow_rate(48*2, 48*4, 48) | ||
| for i in range(3): | ||
| self.p300.aspirate(200, Purification_Beads) | ||
| self.p300.dispense(200, DNA_pool) | ||
| self.p300.aspirate(120, Purification_Beads) | ||
| self.p300.dispense(120, DNA_pool) | ||
| # mix beads well with pool and incubate | ||
| self.set_flow_rate(48*4, 48*6, 48) | ||
| self.p300.mix(10, 200, DNA_pool.bottom(z=10.0)) | ||
| self.set_flow_rate(48*4, 48*4, 48) | ||
| for i in range(20): | ||
| self.p300.aspirate(200, DNA_pool.bottom(z=10.0)) | ||
| self.p300.dispense(200, DNA_pool.center()) | ||
| self.set_flow_rate(48*4, 48*6, 48/6) | ||
| self.p300.mix(20, 200, DNA_pool.bottom(z=10.0)) | ||
| self.p300.move_to(DNA_pool.top()) | ||
| self.protocol.delay(seconds=5.0) | ||
| self.p300.blow_out(DNA_pool.top()) | ||
| self.p300.drop_tip() | ||
| self.protocol.delay(minutes=10) | ||
| self.mag_mod.engage() | ||
| self.protocol.delay(minutes=12) |
There was a problem hiding this comment.
Hmmmm I'm not sure I agree here. If I'm a new developer to this file, I would much rather look at the Pool() function and see:
incubate_purification_beads()
discard_supernatant()
ethanol_wash()
resuspend_beads()
...
etc. than a 500 line function. That way I can drop into just the function that is relevant to what I'm looking at
| self.p300.aspirate(200, DNA_pool.bottom(z=0.5)) | ||
| self.p300.dispense(200, Purification_Beads_discard) | ||
| self.p300.move_to(Purification_Beads_discard.top(z=10.0)) | ||
| self.protocol.delay(minutes=2) |
There was a problem hiding this comment.
Hmmm I think that function is much more readable than 40 lines of very similar looking text. You can remove the has_delay argument and just set delay to -1 if you want no delay if you wanted to make it a little cleaner
| self.set_flow_rate(48, 48, 48) | ||
| self.p300.pick_up_tip() | ||
| for i in range(6): | ||
| self.p300.aspirate(200, Ethanol[1]) | ||
| self.p300.dispense(200, DNA_pool.top(z=-10.0)) | ||
| self.p300.move_to(DNA_pool.top(z=10.0)) | ||
| self.protocol.delay(seconds=40) | ||
| self.discard_supernatant(1200, DNA_pool, Ethanol_discard[1].top(z=-10.0)) |
There was a problem hiding this comment.
I actually think this shouldn't be a function, but should be a for loop over zip(Ethanol, Ethanol_discard). I think that's much more readable than trying to understand what the differences between the times are.
|
@bryanliujiang to add comments throughout the codebase about what things are Octant specific parameters and could be varied in further development |
This is the full version of the OT-2 protocol for library prep that is currently being used at Octant. A write-up for adapting the workflow around the OT-2 will be posted later on.
After running the main thermocycler protocol in "A Reaction" and adding EDTA, the samples can thereafter be processed by the OT-2 for the rest of the Twist 96-Plex Library Preparation Protocol up to final validation/quantification for sequencer loading.
Points to note:
ssh_modevariable toFalse.set_offset()method.Pool()andPost_Pool()stages, the labware loaded on the Magnetic Module changes (from a NEST 96 2mL deep-well plate to a Bio-Rad 96 skirted PCR plate). The App can only calibrate one of these labware at a time. To ensure proper labware offset calibration, it is recommended to split this "full" protocol into two: one for just thePool()stage and the other for just thePost-Pool()stage.