Skip to content

Commit

Permalink
Merge pull request #4398 from sanger/develop
Browse files Browse the repository at this point in the history 
Develop into master release fix
  • Loading branch information
@yoldas
yoldas authored Oct 7, 2024
2 parents 6343546 + 8e1d1eb commit aa375de
Show file tree
Hide file tree
Showing 7 changed files with 27 additions and 227 deletions.
2 changes: 1 addition & 1 deletion .release-version
View file
Original file line number Diff line number Diff line change
@@ -1 +1 @@
14.43.2
14.43.3
35 changes: 6 additions & 29 deletions app/models/cherrypick_task.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,16 +21,8 @@ class CherrypickTask < Task # rubocop:todo Metrics/ClassLength
#
# @return [CherrypickTask::ControlLocator] A generator of control locations
#

def new_control_locator(params)
CherrypickTask::ControlLocator.new(
batch_id: params[:batch_id],
total_wells: params[:total_wells],
num_control_wells: params[:num_control_wells],
wells_to_leave_free: params[:wells_to_leave_free] || DEFAULT_WELLS_TO_LEAVE_FREE,
control_source_plate: params[:control_source_plate],
template: params[:template]
)
def new_control_locator(batch_id, total_wells, num_control_wells, wells_to_leave_free: DEFAULT_WELLS_TO_LEAVE_FREE)
CherrypickTask::ControlLocator.new(batch_id:, total_wells:, num_control_wells:, wells_to_leave_free:)
end

#
Expand All @@ -46,7 +38,7 @@ def can_link_directly?
# rubocop:todo Metrics/ParameterLists
def pick_new_plate(requests, template, robot, plate_purpose, control_source_plate = nil, workflow_controller = nil)
target_type = PickTarget.for(plate_purpose)
perform_pick(requests, robot, control_source_plate, workflow_controller, template) do
perform_pick(requests, robot, control_source_plate, workflow_controller) do
target_type.new(template, plate_purpose.try(:asset_shape))
end
end
Expand All @@ -62,7 +54,7 @@ def pick_onto_partial_plate(
purpose = partial_plate.plate_purpose
target_type = PickTarget.for(purpose)

perform_pick(requests, robot, control_source_plate, workflow_controller, template) do
perform_pick(requests, robot, control_source_plate, workflow_controller) do
target_type
.new(template, purpose.try(:asset_shape), partial_plate)
.tap do
Expand All @@ -74,7 +66,7 @@ def pick_onto_partial_plate(
# rubocop:enable Metrics/ParameterLists

# rubocop:todo Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity, Metrics/MethodLength
def perform_pick(requests, robot, control_source_plate, workflow_controller, template) # rubocop:todo Metrics/AbcSize
def perform_pick(requests, robot, control_source_plate, workflow_controller) # rubocop:todo Metrics/AbcSize
max_plates = robot.max_beds
raise StandardError, 'The chosen robot has no beds!' if max_plates.zero?

Expand All @@ -88,22 +80,7 @@ def perform_pick(requests, robot, control_source_plate, workflow_controller, tem
num_plate = 0
batch = requests.first.batch
control_assets = control_source_plate.wells.joins(:samples)
control_locator =
new_control_locator(
{
batch_id: batch.id,
total_wells: current_destination_plate.size,
num_control_wells: control_assets.count,
template: template,
control_source_plate: control_source_plate
}
)

if control_locator.handle_incompatible_plates
message = 'The control plate and plate template are incompatible'
workflow_controller.send(:flash)[:error] = message unless workflow_controller.nil?
workflow_controller.redirect_to action: 'stage', batch_id: batch.id, workflow_id: workflow.id
end
control_locator = new_control_locator(batch.id, current_destination_plate.size, control_assets.count)
control_posns = control_locator.control_positions(num_plate)

# If is an incomplete plate, or a plate with a template applied, copy all the controls missing into the
Expand Down
100 changes: 15 additions & 85 deletions app/models/cherrypick_task/control_locator.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,34 +29,23 @@ class CherrypickTask::ControlLocator
# limit ourself to primes to simplify validation.
BETWEEN_PLATE_OFFSETS = [53, 59].freeze

attr_reader :batch_id,
:total_wells,
:wells_to_leave_free,
:num_control_wells,
:available_positions,
:control_source_plate
attr_reader :batch_id, :total_wells, :wells_to_leave_free, :num_control_wells, :available_positions

# @note wells_to_leave_free was originally hardcoded for 96 well plates at 24, in order to avoid
# control wells being missed in cDNA quant QC. This requirement was removed in
# https://github.com/sanger/sequencescape/issues/2967 however I've avoided stripping out the behaviour
# completely in case controls are used in other pipelines.
#
# @param params [Hash] A hash containing the following keys:
# - :batch_id [Integer] The id of the batch, used to generate a starting position
# - :total_wells [Integer] The total number of wells on the plate
# - :num_control_wells [Integer] The number of control wells to lay out
# - :wells_to_leave_free [Enumerable] Array or range indicating the wells to leave free from controls
# - :control_source_plate [ControlPlate] The plate to source controls from
# - :template [PlateTemplate] The template of the destination plate

def initialize(params)
@batch_id = params[:batch_id]
@total_wells = params[:total_wells]
@num_control_wells = params[:num_control_wells]
@wells_to_leave_free = params[:wells_to_leave_free].to_a || []
@available_positions = (0...@total_wells).to_a - @wells_to_leave_free
@control_source_plate = params[:control_source_plate]
@plate_template = params[:template]
# @param batch_id [Integer] The id of the batch, used to generate a starting position
# @param total_wells [Integer] The total number of wells on the plate
# @param num_control_wells [Integer] The number of control wells to lay out
# @param wells_to_leave_free [Enumerable] Array or range indicating the wells to leave free from controls
def initialize(batch_id:, total_wells:, num_control_wells:, wells_to_leave_free: [])
@batch_id = batch_id
@total_wells = total_wells
@num_control_wells = num_control_wells
@wells_to_leave_free = wells_to_leave_free.to_a
@available_positions = (0...total_wells).to_a - @wells_to_leave_free
end

#
Expand All @@ -66,7 +55,7 @@ def initialize(params)
# @param num_plate [Integer] The plate number within the batch
#
# @return [Array<Integer>] The indexes of the control well positions

#
def control_positions(num_plate)
raise StandardError, 'More controls than free wells' if num_control_wells > total_available_positions

Expand All @@ -76,25 +65,9 @@ def control_positions(num_plate)

# If num plate is equal to the available positions, the cycle is going to be repeated.
# To avoid it, every num_plate=available_positions we start a new cycle with a new seed.

placement_type = control_placement_type
if placement_type.nil? || %w[fixed random].exclude?(placement_type)
raise StandardError, 'Control placement type is not set or is invalid'
end

handle_control_placement_type(placement_type, num_plate)
end

def handle_incompatible_plates
return false if control_placement_type == 'random'
return false if @plate_template.wells.empty?

control_assets = control_source_plate.wells.joins(:samples)

converted_control_assets = convert_assets(control_assets.map(&:map_id))
converted_template_assets = convert_assets(@plate_template.wells.map(&:map_id))

converted_control_assets.intersect?(converted_template_assets)
seed = seed_for(num_plate)
initial_positions = random_positions_from_available(seed)
control_positions_for_plate(num_plate, initial_positions)
end

private
Expand Down Expand Up @@ -123,49 +96,6 @@ def random_positions_from_available(seed)
available_positions.sample(num_control_wells, random: Random.new(seed))
end

def control_placement_type
@control_source_plate.custom_metadatum_collection.metadata['control_placement_type']
end

def handle_control_placement_type(placement_type, num_plate)
if placement_type == 'random'
control_positions_for_plate(num_plate, random_positions_from_available(seed_for(num_plate)))
else
fixed_positions_from_available
end
end

# Because the control source plate wells are ordered inversely to the destination plate wells,
# the control asset ids need to be converted to the corresponding destination plate well indexes.

def convert_assets(control_assets)
valid_map, invalid_map = create_plate_maps

control_assets.map do |id|
invalid_location = valid_map[id]
invalid_map.key(invalid_location) - 1
end
end

def fixed_positions_from_available
control_assets = @control_source_plate.wells.joins(:samples)
control_wells = control_assets.map(&:map_id)
convert_assets(control_wells)
end

# The invalid and valid maps are hash maps to represent a plate that maps A1 -> 1, A2 -> 2, etc,
# whereas the other map is the inverse of this, mapping 1 -> A1, 2 -> B1, etc.

def create_plate_maps
rows = ('A'..'H').to_a
columns = (1..12).to_a

valid_map = rows.product(columns).each_with_index.to_h { |(row, col), i| [i + 1, "#{row}#{col}"] }
invalid_map = columns.product(rows).each_with_index.to_h { |(col, row), i| [i + 1, "#{row}#{col}"] }

[valid_map, invalid_map]
end

# Works out which offset to use based on the number of available wells and ensures we use
# all wells before looping. Will select the first suitable value from BETWEEN_PLATE_OFFSETS
# excluding any numbers that are a factor of the available wells. In the incredibly unlikely
Expand Down
11 changes: 2 additions & 9 deletions app/views/workflows/_plate_template_batches.html.erb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,15 +19,8 @@
<%= select(:plate_template, "0", @plate_templates.map { |pt| [pt.name, pt.id ] }, {}, class: 'form-control select2') %>
<p class='help-block'>Templates define which wells to leave empty on a plate when you cherrypick samples. You can add to an existing partial plate by scanning the barcode, or entering the plate ID. The plate must have been previously picked in Sequencescape. Wells can be rearranged in the next step.</p>

<label for="Control_plate_id">Control plate & placement type</label>
<%= select("Control", "plate_id",
ControlPlate.all.collect do |p|
placement_type = p.custom_metadatum_collection&.metadata&.[]('control_placement_type')
if placement_type.present?
[ "#{p.human_barcode} - #{p.name} (#{placement_type.capitalize})", p.id ]
end
end.compact,
{ include_blank: true }, class: 'form-control select2') %>
<label for="Control_plate_id">Control plate</label>
<%= select("Control", "plate_id", ControlPlate.all.collect {|p| [ "#{p.human_barcode} - #{p.name}", p.id ] }, { include_blank: true }, class: 'form-control select2') %>
</fieldset>
</div>

Expand Down
10 changes: 0 additions & 10 deletions spec/factories/plate_factories.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -242,16 +242,6 @@
transient { well_factory { :untagged_well } }

after(:create) do |plate, _evaluator|
custom_metadatum = CustomMetadatum.new
custom_metadatum.key = 'control_placement_type'
custom_metadatum.value = 'random'
custom_metadatum_collection = CustomMetadatumCollection.new
custom_metadatum_collection.custom_metadata = [custom_metadatum]
custom_metadatum_collection.asset = plate
custom_metadatum_collection.user = User.new(id: 1)
custom_metadatum_collection.save!
custom_metadatum.save!

plate.wells.each_with_index do |well, index|
next if well.aliquots.empty?

Expand Down
88 changes: 2 additions & 86 deletions spec/models/cherrypick_task/control_locator_spec.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,16 +3,7 @@
require 'rails_helper'

RSpec.describe CherrypickTask::ControlLocator do
let(:instance) do
described_class.new(
batch_id: batch_id,
total_wells: total_wells,
num_control_wells: num_control_wells,
wells_to_leave_free: wells_to_leave_free,
control_source_plate: create(:control_plate),
template: create(:plate_template_with_well)
)
end
let(:instance) { described_class.new(batch_id:, total_wells:, num_control_wells:, wells_to_leave_free:) }

shared_examples 'an invalid ControlLocator' do |plate_number, error = 'More controls than free wells'|
it 'throws a "More controls than free wells" exception' do
Expand Down Expand Up @@ -59,11 +50,6 @@
this_plate.each_with_index { |position, index| expect(position).not_to be_within(5).of(previous_plate[index]) }
end
end

it 'uses a control plate that is valid' do
placement_type = instance.send(:control_placement_type)
expect(placement_type.nil? || %w[fixed random].exclude?(placement_type)).to be_falsey
end
end

# Control positions will be our only public method, sand perhaps some attr_readers
Expand Down Expand Up @@ -140,16 +126,7 @@
let(:range) { (1...1000) }
let(:control_positions) do
range.map do |batch_id|
described_class
.new(
batch_id: batch_id,
total_wells: 96,
num_control_wells: 1,
control_source_plate: create(:control_plate),
template: create(:plate_template)
)
.control_positions(0)
.first
described_class.new(batch_id: batch_id, total_wells: 96, num_control_wells: 1).control_positions(0).first
end
end

Expand All @@ -168,66 +145,5 @@
expect(tally.values).to all be_between(2, 25)
end
end

context 'when the control placement type is not valid' do
let(:batch_id) { 1 }
let(:total_wells) { 96 }
let(:num_control_wells) { 2 }
let(:wells_to_leave_free) { [] }

before { allow(instance).to receive(:control_placement_type).and_return('invalid_type') }

it 'raises an error about invalid placement type' do
expect { instance.control_positions(0) }.to raise_error(
StandardError,
'Control placement type is not set or is invalid'
)
end
end

context 'when the control plate and plate template are incompatible' do
let(:batch_id) { 1 }
let(:total_wells) { 96 }
let(:num_control_wells) { 2 }
let(:wells_to_leave_free) { [] }

before do
allow(instance).to receive_messages(
control_placement_type: 'fixed',
convert_assets: [1, 2, 3],
control_source_plate: create(:control_plate)
)
end

it 'returns and displays an error message' do
expect(instance.handle_incompatible_plates).to be_truthy
end
end

context 'when assets are converted using the maps' do
let(:batch_id) { 1 }
let(:total_wells) { 96 }
let(:num_control_wells) { 2 }
let(:wells_to_leave_free) { [] }

before { allow(instance).to receive_messages(control_placement_type: 'fixed') }

it 'they are given the correct position IDs' do
expect(instance.send(:convert_assets, [94, 95, 96])).to eq([79, 87, 95])
end
end

context 'when the control placement type is fixed' do
let(:batch_id) { 1 }
let(:total_wells) { 96 }
let(:num_control_wells) { 2 }
let(:wells_to_leave_free) { [] }

before { allow(instance).to receive_messages(control_placement_type: 'fixed') }

it 'passes as intended' do
expect(instance.send(:fixed_positions_from_available)).to eq([])
end
end
end
end
8 changes: 1 addition & 7 deletions spec/models/tasks/cherrypick_task_spec.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,11 +36,8 @@ def requests_for_plate(plate)
batch_id: 1235,
total_wells: 6,
num_control_wells: 2,
wells_to_leave_free: wells_to_leave_free,
control_source_plate: control_plate,
template: template
wells_to_leave_free: wells_to_leave_free
).and_return(locator)
allow(locator).to receive(:handle_incompatible_plates)
end

let(:instance) { described_class.new }
Expand Down Expand Up @@ -92,7 +89,6 @@ def requests_for_plate(plate)
locator = instance_double(CherrypickTask::ControlLocator)
allow(locator).to receive(:control_positions).and_return([2, 5], [0, 2])
allow(CherrypickTask::ControlLocator).to receive(:new).and_return(locator)
allow(locator).to receive(:handle_incompatible_plates)
end

it 'places controls in a different position' do
Expand Down Expand Up @@ -121,7 +117,6 @@ def requests_for_plate(plate)
before do
locator = instance_double(CherrypickTask::ControlLocator, control_positions: [2, 3])
allow(CherrypickTask::ControlLocator).to receive(:new).and_return(locator)
allow(locator).to receive(:handle_incompatible_plates)
end

let(:instance) { described_class.new }
Expand Down Expand Up @@ -150,7 +145,6 @@ def requests_for_plate(plate)
locator = instance_double(CherrypickTask::ControlLocator)
allow(locator).to receive(:control_positions).and_return([2, 4], [0, 2])
allow(CherrypickTask::ControlLocator).to receive(:new).and_return(locator)
allow(locator).to receive(:handle_incompatible_plates)
end

let(:instance) { described_class.new }
Expand Down

0 comments on commit aa375de

Please sign in to comment.