[lucaspatel]

Hi Amanda — this PR fixes a bug in plate compression that can put sample names on the wrong wells without any error. I found it while going through the amplicon notebook step by step for the Qiita-MIINT library prep work that follows #369.

When a TubeCode on the plate appears in more than one row of the sample accession file, the left merge in _merge_accession_to_compressed_plate_df adds a row. The update() that follows aligns rows by position, so from that well onward every sample name shifts down by one. I reproduced this on the existing test fixtures, with the KatharoSeq tubes removed to match a typical amplicon plate:

• Same TubeCode under two different sample names: validate_plate_df passes, and 357 of 384 wells end up with the wrong name.
• Exact duplicate row: it gets caught, but as "The following sample names are duplicated", which points at the wrong cause.
• Plates with KatharoSeq: it's only caught by accident, because the shift breaks the dilution count ("only has 7").

add_controls has the same problem. If a blank or KatharoSeq tube is listed twice, for example a blanks scan saved twice in the blanks directory, its well appears twice in the output.

Both merges now raise a ValueError before merging. The error names each ambiguous TubeCode and the values it maps to, e.g.:

The following TubeCodes appear more than once in the sample accession file: 363132553 (41B.Month6.1, 41B.Month6.2)

A duplicate TubeCode that isn't on the plate is still ignored, since it can't affect the merge. That keeps large multi-study accession files with an unrelated duplicate working as before.

There are four new tests in test_metapool.py, all using existing fixtures:

• one TubeCode with two sample names;
• an exact duplicate row;
• a blanks file saved twice;
• a duplicate that isn't on the plate, whose output must still match the existing compression fixture.

The first three fail on master. On Python 3.10 / pandas 2.3 the compress, add_controls and validate tests pass and flake8 is clean. The full suite has no failures beyond 7 that also fail on a clean master checkout here (seqpro and test_count_collector). This also merges cleanly with #368, and test_metapool.py passes on the merged tree under pandas 3.

Since the notebooks call compress_plates and add_controls, they need no changes. The fix covers the amplicon, metagenomics compression and TellSeq notebooks.

Best,
Lucas

[Copilot]

🟡 Changes recommended

Duplicate missing TubeCodes still bypass validation and can corrupt sample assignments or duplicate wells.

1 open issue

• Handle missing TubeCodes to prevent duplicate merge assignments · New

What changed in this PR

Prevents ambiguous TubeCodes from corrupting plate assignments. The diff also includes Python 3.11/pandas 3 migration work related to #368.

Changes:

• Adds duplicate TubeCode checks before accession and controls merges.
• Updates pandas compatibility and missing-value handling.
• Adds regression tests and updates notebooks and test infrastructure.

File	Description
setup.py	Requires Python 3.11 and pandas 3.
notebooks/​tellseq_D_variable_volume_pooling.ipynb	Uses shared read-count splitting.
notebooks/​metatranscriptomics_matrix_pipeline_seqcount_norm.ipynb	Updates string conversion and read-count splitting.
notebooks/​matrix_tube_pipeline_seqcount_norm.ipynb	Uses shared read-count splitting.
notebooks/​amplicon_pre_prep_file_generator.ipynb	Explicitly checks index uniqueness.
metapool/​util.py	Adds dtype-aware placeholders and unique indexing.
metapool/​tests/​test_util.py	Tests unique indexing.
metapool/​tests/​test_sample_sheet.py	Tests missing-value handling.
metapool/​tests/​test_plate.py	Covers empty replication columns and index compatibility.
metapool/​tests/​test_metapool.py	Tests duplicate TubeCodes and read-count validation.
metapool/​scripts/​tests/​test_seqpro.py	Updates temporary directories and path assertions.
metapool/​scripts/​tests/​test_seqpro_mf.py	Updates temporary directories and path assertions.
metapool/​sample_sheet.py	Improves missing-value serialization and validation.
metapool/​prep.py	Uses shared unique indexing.
metapool/​plate.py	Handles replication column types.
metapool/​metapool.py	Adds merge guards and pandas compatibility changes.
metapool/​count.py	Updates indexing and string conversions.
metapool/​abs_quant_sample_info.py	Updates lot-ID string conversion.
environment.yml	Targets Python 3.11 and pandas 3.
conftest.py	Uses non-interactive plotting for tests.
.github/​workflows/​deploy.yml	Targets Python 3.11.
.github/​workflows/​ci.yml	Targets Python 3.11.

_{Review effort: Balanced}

[AmandaBirmingham]

Below is from Claude. Note from Amanda: I threw out about half Claude's findings or proposed fixes as not worth it; these are the ones that seemed either trivial to fix or worth the (probably still pretty minimal) Claude test-writing effort.

Review: PR #370 (duplicate TubeCode guard), plus follow-ups from #368

Nothing below is blocking. Part A covers this PR. Part B covers code that came in with #368, which is already merged, so those items are follow-ups rather than changes to this PR. Worth noting that copilot also flags A2.

Line numbers refer to the PR merged onto current master.

Part A: this PR (#370)

A1. The compress-plates error blames "the sample accession file" for duplicates that file doesn't contain

metapool/metapool.py:2170-2172

extend_sample_accession_df (metapool/util.py:136-138) strips the qiita_id. prefix and merges on sample_name. If two studies share a sample name, each accession row with that name is duplicated, once per project. All three main compression notebooks pass that extended frame into compress_plates, and the guard then reports a duplicate "in the sample accession file", which has none:

metadata adds 14577.41B.Month6.1 alongside 12986.41B.Month6.1
extended rows: 3942 vs file rows: 3941 ; dup TubeCodes in file: 0
The following TubeCodes appear more than once in the sample accession file:
  363132553 (41B.Month6.1 / Celeste_Adaptation_12986, 41B.Month6.1 / Celeste_Marmoset_14577)

Catching this is still an improvement: before the guard, the wells shifted silently.

Suggested fix: change the source wording to "the sample accession data" (two test regexes pin the current text).

A2. Missing TubeCodes are excluded from the check, but the merge still duplicates them

metapool/metapool.py:2210

The guard drops missing TubeCodes (& lookup_df[TUBECODE_KEY].notna()), but a pandas left merge matches a missing key to a missing key (confirmed under pandas 3.0.6). If the plate has an empty TubeCode cell and the accession file has two blank-TubeCode rows, that well gets two rows. The index-aligned update() then shifts later sample names onto the wrong wells, with no error. The same applies in add_controls. This predates the PR, but the guard explicitly lets it through, and no test covers it either way (removing .notna() still passes all tests).

Suggested fix: either reject blank TubeCodes in the lookup data or drop them before the merge, and add a test for whichever is chosen.

A3. No test checks that the add_controls guard runs after leading zeroes are stripped

metapool/metapool.py:2311-2316

Moving the guard above strip_tubecode_leading_zeroes still passes all tests, because the new test uses a plate that is already stripped. With a plate that still has leading zeroes, that order would find no matches, so 15 real duplicates would pass silently and the merge would add 15 rows.

Suggested fix: add a test that uses comp_plate_exp_fp_w_leading_zeroes_tubecode with the doubled blanks directory and preserve_leading_zeroes=False, expecting the ValueError.

A4. The new compress tests only cover exact duplicates, which the notebooks already reject on load

metapool/tests/test_metapool.py:235, :251

The notebooks load accession files through join_dfs_from_files, which already rejects an exact repeated TubeCode such as 0363132553 twice. In those flows the guard only adds protection for collisions that appear after normalization (e.g. 0363132553 and 363132553), and for the cross-study case in A1. Neither has a test.

Suggested fix: add a test for the leading-zero collision:

sa_df + {'other', '363132553'} (file already has '0363132553')
-> raise: ... 363132553 (41B.Month6.1, other)
control, preserve_leading_zeroes=True -> no raise

A one-line comment at the guard saying it covers duplicates that only appear after normalization would also explain why two TubeCode checks exist.

A5. The controls error message is noisy

metapool/metapool.py:2217

controls_df concatenates the blanks and katharoseq frames, so every blank row has missing Kathseq_RackID and number_of_cells. With the same blanks scan saved twice, the message is 1,156 characters long, made of 15 entries like:

363183400 (negative_control / nan / nan, negative_control / nan / nan)

Suggested fix: skip missing values when joining the details (e.g. " / ".join(str(v) for v in row if pd.notna(v))).

A6. Nit: the order of a multi-TubeCode error message isn't pinned

The only multi-TubeCode test asserts a regex that ends at ": ", so changing sort=True to sort=False in the groupby still passes. This only matters if the order is meant to be part of the message's contract.

Part B: from #368 (already merged; follow-ups)

B1. Missing-sample-name errors report index labels as "row(s)"

metapool/metapool.py:1500-1508

The message says "at row(s): N", but N comes from counts_df.index labels. A blank on file line 3 is reported as "row(s): 1". On a filtered or concatenated frame, the label may not match any position. The Sample_ID check added in the same PR (metapool/sample_sheet.py:1062-1067) counts sample rows from 1, so users see two numbering schemes.

Suggested fix: Keep the labels and word it "index label(s)". Update the assertions at test_metapool.py:1639 and :1658 to match.

B2. The PM_SAMPLE_KEY entry in make_replicates isn't tested

metapool/plate.py:969-976; test at metapool/tests/test_plate.py:935-945

The new test's subtests cover orig_name, Library Well and replicate, but not PM_SAMPLE_KEY. Removing it from the tuple still passes all tests, yet it is needed:

int Sample, real code         -> OK, Sample dtype str
int Sample, PM_SAMPLE_KEY dropped -> TypeError Invalid value '1000.A1' for dtype 'int64'

The loop also converts whole columns to str, including rows that aren't replicated. That's low impact, since read_plate_map_csv already reads Sample as str.

Suggested fix: add a PM_SAMPLE_KEY case with integer names. The comment could also say it covers any non-text column, not only all-empty ones.

B3. No test covers whitespace-only sample names in the missing-name check

metapool/metapool.py:1504

Reducing the check to missing = names.isna() still passes all tests. That case can happen: read_csv(..., dtype=str) keeps " " as ' ' rather than NaN.

Suggested fix: add a test with a whitespace-only name.

B4. split_trimmed_read_counts negates str.contains without converting to text

metapool/metapool.py:1520

On an object column that holds a non-string value, str.contains returns NaN for that row and ~trimmed_mask raises:

object col ['a.trimmed.fq', 'b.fq', 123] -> TypeError bad operand type for unary ~: 'float'
all-int col [1, 2]                       -> AttributeError Can only use .str accessor with string values

The notebook read paths produce strings, so this is low severity and not a regression.

Suggested fix: read_counts_df[sample_column].astype(str).str.contains('trimmed'). This is safe because missing names are already rejected earlier. na=False alone isn't a good fix: it would silently classify a non-string name as raw.

B5. The amplicon notebook repeats set_unique_index inline

notebooks/amplicon_pre_prep_file_generator.ipynb

The notebook does set_index, checks is_unique and builds the duplicate message itself, copying the "Index has duplicate keys" text from metapool/util.py:234. If the helper's logic or message changes, the notebook won't follow.

Suggested fix: add from metapool.util import set_unique_index (it isn't in scope via from metapool.metapool import *) and replace the inline code with qiita_df = set_unique_index(qiita_df, 'sample_name').

B6. Nit: test_metapool.py imports both io and StringIO

metapool/tests/test_metapool.py:2, :7

io. is used once (io.StringIO in test_split_trimmed_read_counts_err_missing_sample_name); the module already imports StringIO directly.

Suggested fix: change that call to StringIO(...) and drop import io.

[lucaspatel]

Thanks for the careful pass (and for pre-filtering Claude's list). Part A is addressed in d6d2c4a. The Part B items are follow-ups to already-merged #368, so they're going up as a separate PR against master rather than here.

Part A

• A1 — wording. The compress guard now says "the sample accession data", not "file", since cross-study extension can duplicate a TubeCode the file holds once. Both pinning regexes updated.
• A2 — missing TubeCodes. Fixed in both compress_plates and add_controls by dropping blank-TubeCode rows from the lookup data before the guard and the merge (new _drop_blank_tubecodes), with tests. Reasoning below.
• A3 — guard-after-strip. Added a test that runs add_controls on a plate that still has leading zeroes with preserve_leading_zeroes=False and a control listed twice, expecting the ValueError — which only holds if the guard runs after the strip.
• A4 — post-normalization duplicates. Added tests for the leading-zero collision (0363132553 vs 363132553) in both the stripped (raises) and preserved (no raise) cases, and a comment on the guard explaining it covers duplicates that only appear after normalization.
• A5 — noisy controls message. The duplicate detail now skips missing values, so a concatenated blanks+katharoseq frame no longer reads nan / nan for every entry.
• A6 — message order. Kept sort=True with a comment pinning the message as deterministic (ordered by TubeCode); didn't add a dedicated order test, but happy to if you'd like the contract enforced.

On A2 (the decision): we drop blank-TubeCode rows from the lookup data before the merge rather than reject them. A blank TubeCode can't identify a well, so those rows had no legitimate way to join on TubeCode — the only thing they did was the bug, where a pandas left merge matched blank-to-blank and duplicated the well (shifting later names via the aligned update()). Dropping them means a blank-TubeCode well now correctly gets no assignment instead of grabbing an arbitrary blank row; no correct assignment changes, since such a row could never be matched by TubeCode anyway. We chose drop over reject because reject would also fail files carrying some legitimately-blank rows, which the rest of the pipeline tolerates. The drop is silent, matching the guard's prior "ignore blanks" behavior — happy to add a one-line warning when rows are dropped if you'd rather see it.