Extend 'under-represented wells' feature to other pipelines by sabrine33 · Pull Request #5769 · sanger/sequencescape

sabrine33 · 2026-05-12T08:29:18Z

update the logic behind construct under_rep comments to include all submission types.
also update the logic behind comparing aliquots
Tested the fix on training on the batches with wrong data (explained here) and it returned the expected/right values.

Closes #

Changes proposed in this pull request

Instructions for Reviewers

[All PRs] - Confirm PR template filled
[Feature Branches] - Review code
[Production Merges to main]
- Check story numbers included
- Check for debug code
- Check version

…ubmission types. also update the logic behind comparing aliquots

codecov · 2026-05-12T08:54:40Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.29%. Comparing base (18aae87) to head (0c9f815).
⚠️ Report is 33 commits behind head on develop.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #5769      +/-   ##
===========================================
- Coverage    87.36%   87.29%   -0.07%     
===========================================
  Files         1477     1477              
  Lines        33526    33535       +9     
  Branches      3550     3552       +2     
===========================================
- Hits         29290    29275      -15     
- Misses        4215     4239      +24     
  Partials        21       21

Flag	Coverage Δ
javascript	`50.00% <ø> (ø)`
ruby	`87.31% <100.00%> (-0.08%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

andrewsparkes

Looks good. Clear comments, thorough tests, thanks.

KatyTaylor

A few things to consider.

Would be good to test it on at least one plate from each of the mentioned pipelines.

KatyTaylor · 2026-05-13T12:24:21Z

  def under_represented_well_comments
-    request_with_under_represented_wells.flat_map do |request|
-      build_comments_for_request(request)
+    requests.flat_map do |batch_request|


Suggested change

requests.flat_map do |batch_request|

requests.flat_map do |sequencing_request|

Think it's confusing to call this variable batch_request since that there is actually a different model / table called batch_request, and this is a request.
(Also applicable to naming in a couple of other methods).

@KatyTaylor - they are actually batch_requests (which can be/usually are sequencing requests). As we are reading from the model Batch:

sequencescape/app/models/batch.rb

Line 44 in fd134ce

has_many :requests, -> { distinct }, through: :batch_requests, inverse_of: :batch

Batch.last.requests.first.class
=> HiSeqSequencingRequest (a type of Request)

Batch.last.batch_requests.first.class
=> BatchRequest (not a type of Request - a join table)

KatyTaylor · 2026-05-13T12:41:34Z

+  def under_rep_requests_for_lane(lane)
+    lane.ancestors.grep(Plate)
+      .flat_map(&:wells)
+      .flat_map(&:requests)


Above line will return duplicate Requests, as multiple Plates in a pipeline point to the same Request. Might also get some nils, as some Plates (e.g. first ones in a Pipeline, or ones that don't require Requests) will not point to a Request. Could do a .uniq.compact

KatyTaylor · 2026-05-13T12:43:30Z

+  # @param batch_request [Request] the sequencing request whose position is used in the comment
+  # @return [Array<UnderRepWellComment>] comments built for the given lane
+  def build_comments_for_lane(lane, batch_request)
+    under_rep_requests_for_lane(lane).flat_map do |library_request|


Could mention in the comment that we are assuming it's a library_request as that is the known use case. The way it's retrieved does not enforce it.

KatyTaylor · 2026-05-13T12:47:58Z

-    request.target_asset.aliquots.filter_map do |aliquot|
-      lane = request.asset.descendants.last
-      next unless aliquot_matches_lane?(lane, aliquot)
+  # @param library_request [Request] the library request whose target_asset well holds the aliquots


This is a limitation in the original design. Since we are storing the polymetadata on request, not well, it only works for plates that have a request 'finishing' in them. If you try to mark a well on an 'intermediate' plate as under-represented, this will assume that it came from a different plate. Think we need to state this explicitly somewhere and maybe code in some guards against it.

KatyTaylor · 2026-05-13T12:53:27Z


      UnderRepWellComment.new(
-        position: lane.source_request.position,
+        position: batch_request.position,


I think this is the Request, not the Batch Request, so you should pass through the Batch Request instead to get the position.

There are actually batch requests type, that is why we can the read the position

There are actually batch requests type, that is why we can the read the position

position can be accessed directly from the sequencing request as it's joined to batch request,
batch.requests.first.position or batch.request.first.batch_request.position

OK I hadn't noticed that Request delegates position to BatchRequest in app/models/batch/request_behaviour.rb, so that's how it works 👍

Just some variable naming to make a bit clearer then.

KatyTaylor · 2026-05-13T12:59:29Z

-  def aliquot_matches_lane?(lane, aliquot)
-    lane.aliquots.any? { |a| a.aliquot_index_value == aliquot.tag.map_id }
+  # @return [Aliquot, nil] the matching lane aliquot, or nil if no match is found
+  def find_matching_lane_aliquot(lane, aliquot)


Could use the equivalent? method on Aliquot. There's also a matches? method, but it allows the upstream aliquot to be untagged, which is less strict.

Co-authored-by: KatyTaylor <kt17@sanger.ac.uk>

sabrine33 · 2026-05-13T13:12:36Z

A few things to consider.

Would be good to test it on at least one plate from each of the mentioned pipelines.

I tested on the RNA pipeline but also with different scenarios

Co-authored-by: KatyTaylor <kt17@sanger.ac.uk>

sabrine33 added 2 commits May 12, 2026 09:28

update the logic behind construct under_rep comments to include all s…

6806092

…ubmission types. also update the logic behind comparing aliquots

linting

3b79d90

sabrine33 linked an issue May 12, 2026 that may be closed by this pull request

Y26-090 - Extend 'under-represented wells' feature to other pipelines #5770

Open

2 tasks

sabrine33 changed the title ~~update the logic behind construct under_rep comments to include all s…~~ Extend 'under-represented wells' feature to other pipelines May 12, 2026

sabrine33 assigned yoldas, andrewsparkes and KatyTaylor and unassigned yoldas, andrewsparkes and KatyTaylor May 12, 2026

sabrine33 requested review from KatyTaylor, andrewsparkes and yoldas May 12, 2026 08:33

andrewsparkes approved these changes May 12, 2026

View reviewed changes

Merge branch 'develop' into Y26-090

de82a8b

KatyTaylor requested changes May 13, 2026

View reviewed changes

Update app/models/under_rep_well_comments_to_broadcast.rb

6430ae2

Co-authored-by: KatyTaylor <kt17@sanger.ac.uk>

Update app/models/under_rep_well_comments_to_broadcast.rb

0c9f815

Co-authored-by: KatyTaylor <kt17@sanger.ac.uk>

	requests.flat_map do \|batch_request\|
	requests.flat_map do \|sequencing_request\|

Conversation

sabrine33 commented May 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes proposed in this pull request

Instructions for Reviewers

Uh oh!

codecov Bot commented May 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

andrewsparkes left a comment

Choose a reason for hiding this comment

Uh oh!

KatyTaylor left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sabrine33 May 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sabrine33 commented May 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sabrine33 commented May 12, 2026 •

edited

Loading

codecov Bot commented May 12, 2026 •

edited

Loading

sabrine33 May 13, 2026 •

edited

Loading