Skip to content

Secondary object - #171

Open
akcd1 wants to merge 49 commits into
mainfrom
secondary_object
Open

Secondary object#171
akcd1 wants to merge 49 commits into
mainfrom
secondary_object

Conversation

@akcd1

@akcd1 akcd1 commented Dec 5, 2025 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Description

Thank you for your contribution to Brieflow!
Please succinctly summarize your proposed change.
What motivated you to make this change?

Added the option to segment ang phenotype additional objects contained within cells.

Please also link to any relevant issues that your code is associated with.

What is the nature of your change?

  • Bug fix (fixes an issue).
  • Enhancement (adds functionality).
  • Breaking change (fix or feature that would cause existing functionality to not work as expected).
  • This change requires a documentation update.

Checklist

Please ensure that all boxes are checked before indicating that a pull request is ready for review.

  • My code follows the conventions of this project.
  • I have updated the pyproject.toml to reflect the change as designated by semantic versioning.
  • I have checked linting and formatting with ruff check and ruff format.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation.
  • My changes generate no new warnings.
  • I have deleted all non-relevant text in this pull request template.

@akcd1
akcd1 requested a review from mat10d December 5, 2025 20:09
@mat10d

mat10d commented Dec 7, 2025

Copy link
Copy Markdown
Collaborator

@akcd1 the tests are failing on something that suggests that you haven't properly rebased main. I would remerge asap.

@akcd1

akcd1 commented Dec 8, 2025

Copy link
Copy Markdown
Collaborator Author

re-based

@mat10d
mat10d marked this pull request as draft March 30, 2026 17:07
acepedadiaz and others added 21 commits April 10, 2026 15:12
acepedadiaz acepedadiaz
initial commit, refactoring second obj segmentation
f260fd6
acepedadiaz acepedadiaz
added thresholding options
bb099af
acepedadiaz acepedadiaz
added visualization of thresholded output
b916c2b
acepedadiaz acepedadiaz
ruff check and format
f7d01cf
acepedadiaz acepedadiaz
adds helper visualization function
760cde0
acepedadiaz acepedadiaz
snakemake integration
ff6b8e4
acepedadiaz acepedadiaz
updated parameters and added scaffold for ML-based segmentation
746c9ab
acepedadiaz acepedadiaz
ruff check and format
d56b0de
acepedadiaz acepedadiaz
code consolidation
deac610
acepedadiaz acepedadiaz
improved documentation
cfe0d56
acepedadiaz acepedadiaz
code consolidation and reformatting
c3d7510
acepedadiaz acepedadiaz
remove foci channel from second_obj pheno extraction, rename foci_cha…
a8a89d4 
…nnel_index elsewhere
acepedadiaz acepedadiaz
fixed bug foci_channel param passing
ff3c704
acepedadiaz acepedadiaz
fixed import of second obj features
a8a6b61
acepedadiaz acepedadiaz
initial commit, refactoring second obj segmentation
6c4cea6
acepedadiaz acepedadiaz
ruff check and format
aca177b
@EdenYifrach edeny
acepedadiaz acepedadiaz
Secondary object ml (#173)
dd3d9fb 
* conditional cytoplasm mask

* initial commit, refactoring second obj segmentation

* added thresholding options

* added visualization of thresholded output

* ruff check and format

* adds helper visualization function

* snakemake integration

* updated parameters and added scaffold for ML-based segmentation

* ruff check and format

* code consolidation

* improved documentation

* code consolidation and reformatting

* remove foci channel from second_obj pheno extraction, rename foci_channel_index elsewhere

* fixed bug foci_channel param passing

* fixed import of second obj features

* conditional cytoplasm mask

* Integrated secondary object segmentation with ml models

* ruff format

* disabled secondary object detection for test data

* consistent naming with config and notebook

* fixed ML parameter passing in script identify_second_objs.py

* fixed general config parameters

---------

Co-authored-by: edeny <edeny@fry.wi.mit.edu>
Co-authored-by: acepedadiaz <acepedadiaz@fry.wi.mit.edu>
acepedadiaz acepedadiaz
fixed cpsam incommpatibility secondary object segmentation
59f644c
@akcd1 acepedadiaz
acepedadiaz acepedadiaz
Secondary obj with custom training of cellpose (#188)
3501c73 
* adds fine-tuning of Cellpose, code consolidation

* optimization of model performance calculations

* added image normalization for improved alignment performance

* updated to _cp nomenclature

* update to merge_phenotype_cp

* added intermediate tsv reporting alignment offsets and confidence

* added intermediate tsv reporting alignment offsets and confidence

* removed params for percentile normalization

* added alignment metrics

* removed percentile normalization, added reporting, normalization bug fixed

* minor formatting

* added alignment metrics

* minor formatting

* added alignment metrics

---------

Co-authored-by: acepedadiaz <acepedadiaz@fry.wi.mit.edu>
Co-authored-by: acepedadiaz <acepedadiaz@c13b1.wi.mit.edu>
acepedadiaz @claude
propagate alignment offsets to final merge via CP feature extraction
f33dd82 
Route per-tile offset_y/offset_x from alignment_metrics TSV through
extract_phenotype_cp so they flow naturally through merge_phenotype_cp
into final_merge with zero merge module changes.

- Add alignment metrics TSV as 5th input to extract_phenotype_cp rule
- Broadcast offset columns to each cell row in extract_phenotype.py
- Fix stitch_phenotype input to use align_phenotype[0] (image only)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
acepedadiaz @claude
add configurable secondary object aggregation into aggregate pipeline
38272f4 
Introduce aggregate_cells_second_objs rule between final_merge and
split_datasets with 4 strategies for handling one-to-many cell-object
relationships: "none", "single", "all", "average".

- New second_obj_utils.py with strategy dispatch and merge logic
- New aggregate_cells_second_objs.py snakemake script
- Conditional DAG: uses aggregate output when second_obj_detection is
  enabled, falls back to final_merge otherwise
- Add cell_summary columns to metadata cols so they are excluded from
  feature aggregation

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@akcd1
akcd1 force-pushed the secondary_object branch from 66c3463 to 38272f4 Compare April 10, 2026 20:27
acepedadiaz and others added 2 commits April 10, 2026 16:43
acepedadiaz @claude
fix test config: disable secondary objects and revert metadata cols
8f907f0 
Test images have no secondary objects. Set second_obj_detection: false
in test config and remove secondary object metadata columns from
cell_data_metadata_cols.tsv since they won't exist in test data.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
acepedadiaz @claude
ruff check and format
6a970a7 
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
acepedadiaz and others added 5 commits April 10, 2026 17:10
acepedadiaz @claude
fix: conditional merge_phenotype input and ruff D301 docstring error
74d8f2c 
- merge_phenotype rule now falls back to extract_phenotype_cp when
  second_obj_detection is disabled, fixing MissingInputException in CI
- Add r-prefix to docstring containing backslashes in segment_cellpose.py

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
acepedadiaz @claude
fix: conditional aggregate targets and ruff D415 docstring
92820e1 
Filter aggregate_cells_second_objs from targets/mappings when
second_obj_detection is disabled, matching the pattern in phenotype
targets. Fixes MissingInputException in CI tests. Also add period
to cellpose_training.py docstring to satisfy ruff D415.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
acepedadiaz @claude
fix: destructure calculate_offsets return tuple in align_cycles
6a4691b 
calculate_offsets was changed to return (offsets, errors) but three
call sites in align_cycles.py were not updated to destructure the
tuple, causing ValueError in apply_offsets when offset elements
were arrays instead of scalars.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
acepedadiaz @claude
fix: make alignment metrics optional in extract_phenotype_minimal
4a9d5a1 
The script was updated to load alignment_metrics from snakemake.input[1],
but the SBS extract_sbs_info rule only provides one input (nuclei tiff).
The phenotype extract_phenotype_info rule provides both. Guard the metrics
loading with a length check so the script works for both callers.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@akcd1 acepedadiaz
@claude acepedadiaz
feat(aggregate): port compartment_combo + aggregate fixes onto second…
1473610 
…ary_object (joint scrubbed) (#222)

* feat(aggregate): add compartment_combo_subset to filter feature columns by compartment

* feat(aggregate): add resolve_aggregate_combos validation helper

* feat(shared): register compartment_combo filename prefix (CmCo-)

* feat(aggregate): add compartment_combo wildcard to aggregate output paths

* feat(aggregate): add compartment_combo wildcard to aggregate rules

* feat(shared): add compartment_combo to bootstrap rule_utils helpers

* feat(cluster): add compartment_combo wildcard to cluster paths and rules

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* feat(shared): propagate compartment_combo through metrics, targets, cluster_eval

* feat(aggregate): apply compartment_combo_subset in split_datasets and read wildcard in bootstrap_gene

* test(small_test_analysis): add compartment_combo to aggregate fixtures

* test(small_test_analysis): add compartment_combo to cluster_combo fixture

* fix(aggregate): preserve int64 plate dtype in aggregate_cells_second_objs

The script cast second_objs_df's plate/well to str, then _aggregate_single
merged that against cells_df (plate as int64) and failed with:
  ValueError: You are trying to merge on int64 and object columns for key 'plate'

Both merge_final.parquet and phenotype_second_objs.parquet have plate as
int64; the str coercion was unnecessary. Compare plate as int and leave
well as its natural string form.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(aggregate): use known compartment prefixes as drop universe

all_compartments_run was derived from the compartments present in the
TSV. Compartments never requested in any combo (e.g. cytoplasm when all
combos only use cell/nucleus/second_obj) were absent from the universe,
so compartment_combo_subset never added them to `excluded` and their
feature columns leaked through into every filtered parquet. Source the
universe from COMPARTMENT_PREFIXES so undeclared compartments get
dropped instead of passed through.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* feat(aggregate): make montage generation opt-out via generate_montages

Read config["aggregate"]["generate_montages"] (default True) in
targets/aggregate.smk and evaluate MONTAGE_TARGETS_ALL to an empty list
when False. This lets users skip prepare_montage_data, generate_montage,
and initiate_montage when montages aren't wanted, without needing to
invoke a different snakemake target. Default preserves existing behavior
for configs that don't set the flag.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(aggregate): harmonize pool schema at align + generate_feature_table

Per-well missing_values_filter drops different columns in different wells
when class composition varies (e.g. Uninfected-only wells vs mixed
Bystander+Infected wells). Downstream pooling via pyarrow's dataset
union then fills those cols with NaN across whole wells, which breaks
centerscale_by_batch (zero-variance batch) and PCA (NaN input).

Add harmonize_pool_schema(paths, metadata_cols, drop_cols_threshold) in
lib/aggregate/filter.py that:
  1. Intersects per-file schemas. Cols present in some but not all files
     are dropped up front and logged per-file so the operator can see
     which wells lacked which columns (the "missing class in a well"
     case the user explicitly wants to tolerate).
  2. Applies drop_cols_threshold at the pool level on the intersected
     col set, matching the convention from missing_values_filter.

Wire into align.py and generate_feature_table.py: call the helper once
to produce a canonical metadata+feature col set, pass it as `columns=`
to every ds.dataset scan (PCA sample, class read, batched transform,
feature-table batch loop). Remove the per-batch .dropna(axis=1) in
align.py's batch loop (redundant; could previously drop inconsistent
col sets across batches and break PCA transform). Add defensive
per-subset row-drop on residual NaN.

Pass config["aggregate"]["drop_cols_threshold"] through as a rule
param to both rules.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(aggregate): plumb perturbation columns through perturbation_score

perturbation_score.py hardcoded "gene_symbol_0", "nontargeting", and
"cell_barcode_0" across prepare_alignment_data, centerscale_on_controls,
and calculate_perturbation_scores — so any config where
perturbation_name_col != "gene_symbol_0" ran the scorer on mismatched
columns and either produced silently-wrong scores or blew up. Under
pandas nullable-string dtypes, (gene_symbol_0 == <sgRNA barcode>)
returned a BooleanArray with pd.NA, and .astype(int) collapsed it to
{0, 1, <NA>} — which LogisticRegression(solver="liblinear") then
rejected as n_classes >= 3.

Plumb perturbation_name_col, perturbation_id_col, control_key,
control_name_col, and batch_cols through perturbation_score and
_process_gene_subset from the caller. calculate_perturbation_scores
now requires perturbation_col (no default) and builds y via
.eq(gene).fillna(False).astype(bool).astype(int) so NaN rows always
map to 0 and the target is strictly binary.

align.py passes the additional config values into perturbation_score in
both the joint and non-joint paths.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(aggregate): dedupe construct_columns when pert_id_col == pert_col

When perturbation_id_col and perturbation_name_col point at the same
column (a valid config when aggregation is at the per-construct level,
e.g. both "cell_barcode_0"), building construct_columns as
[pert_id_col, pert_col, "cell_count"] + feature_cols listed that name
twice. construct_table[construct_columns] then produced duplicate
columns, and construct_table[pert_col] returned a 2-col DataFrame,
breaking the subsequent .str.contains(control_key) call with
AttributeError: 'DataFrame' object has no attribute 'str'.

Use dict.fromkeys to dedupe while preserving order.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(aggregate): use .loc instead of .iloc for keep_idx in perturbation_score

keep_idx is built from perturbation_col.index[...] and nt_idx which hold
pandas label indices, but line 102 used .iloc (positional). When
_compute_perturbation_score_joint in align.py iterates groupby(\"class\")
slices without reset_index, cell_data's labels exceed its length and
.iloc raises IndexError: positional indexers are out-of-bounds. Lines
130-131 already use .loc correctly with the same gene_idx; make 102
consistent so the function works with either RangeIndex or preserved
label index inputs.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(aggregate): make eval_aggregate compartment-aware and NaN-safe

eval_aggregate.py hardcoded a cell→nucleus fallback chain when picking
*_mean feature columns for the distribution plot. For combos whose
compartment_combo is second_obj or cytoplasm, neither 'cell_*_mean' nor
'nucleus_*_mean' columns survive compartment_combo_subset, so
merge_feature_cols collapsed to [], and the downstream
plt.subplots(nrows=0, ...) raised ValueError.

Two layers:
- Script: derive the prefix priority from snakemake.wildcards.compartment_combo
  and iterate all known compartments (cell, nucleus, cytoplasm, second_obj)
  as a fallback chain, using the first non-empty set of *_mean columns.
- Lib plot_feature_distributions: guard empty inputs by returning a small
  placeholder figure with an explanatory label. eval_aggregate's other
  outputs (na_stats tsv/png) still write; the DAG proceeds.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* feat(aggregate): carry secondary name column through aggregation

When aggregate is grouped by a construct ID (perturbation_name_col =
cell_barcode_0), the aggregated.tsv previously had no gene_symbol_0
column. Downstream consumers break: cluster benchmarking
(evaluate_resolution -> filter_complexes) tries to intersect
aggregated perturbation names with CORUM/KEGG gene_name fields and
finds nothing; uniprot merge in notebook 10 cell 20 fails on missing
gene_symbol_0. Both assume gene-level naming.

Add a carry_cols parameter to lib/aggregate/aggregate.py:aggregate()
that preserves columns functionally determined by group_cols in the
output — taking the single unique value per group and raising
ValueError if the invariant is violated. Wire the rule aggregate
to pass carry_cols=[control_name_col] when control_name_col is
different from perturbation_name_col (the normal setup for
barcode-level aggregation + gene-level control naming).

Net effect: when pert=cell_barcode_0 and control=gene_symbol_0,
the aggregated.tsv gains a gene_symbol_0 column and downstream
clustering/benchmarking/annotation all get the name they expect.
When pert == control (gene-level aggregation), carry_cols stays
None and behavior is unchanged.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(aggregate): use positional indexing in missing_values_filter imputation

The imputation block read/wrote with .loc[batch_na_idx, cols], which fails
on inputs whose index has duplicate labels. Production callers always feed
unique-indexed frames (scripts/aggregate/filter.py uses ignore_index=True),
but any new caller that doesn't can trip a 'cannot reindex on an axis with
duplicate labels' ValueError. Switch to positional iloc indexing — same
behavior on clean inputs, robust to duplicate-indexed inputs.

* fix(aggregate): plumb control_name_col through generate_feature_table

centerscale_on_controls and the construct/gene control-table masks identified
controls via perturbation_name_col. When perturbation_name_col != control_name_col
(construct-level aggregation, e.g. cell_barcode_0 perturbations with gene_symbol_0
controls) no controls were found: centering-on-controls was silently skipped and
the control gene table was empty. Carry control_name_col through the construct
accumulation and use it for control identification, mirroring align.py. Falls back
to perturbation_name_col when unset, so gene-level runs are unchanged.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* refactor(aggregate): add type hints + pin schema in harmonize_pool_schema

- Pass schema=pq.read_schema(paths[0]) to ds.dataset() in harmonize_pool_schema so
  files are read against one reference schema instead of pyarrow auto-unifying
  (which raises ArrowInvalid on dtype mismatch across pooled per-well parquets).
- Add type hints to harmonize_pool_schema, compartment_combo_subset, and
  resolve_aggregate_combos for consistency with other functions in the PR.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: acepedadiaz <acepedadiaz@cheeserind.wi.mit.edu>
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-authored-by: acepedadiaz <acepedadiaz@wi.mit.edu>
@akcd1
akcd1 marked this pull request as ready for review June 9, 2026 15:46
mat10d and others added 11 commits July 10, 2026 12:29
@mat10d @claude
chore: remove committed test file (house rule: no tests in PRs)
d086f75 
tests/test_compartment_filter.py was committed with the feature; brieflow's test
harness is tests/small_test_analysis/, not standalone pytest files. Removing it.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@mat10d @claude
style: collapse multiline comment stanzas to single lines
d22f53f 
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@mat10d @claude
chore: remove cruft (unused imports, dead code, debug prints)
f43a39d 
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@mat10d @claude
style: Google-style docstrings + public-first lib ordering
9d65852 
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@mat10d @claude
fix(cellpose): augment in the image's native value range, not [0,1]
32ab800 
Intensity-scaling/noise augmentation clipped uint8 [0,255] training images to
[0,1] and cast to float32, collapsing them to near-binary and mixing dtypes with
the (uint8) originals — corrupting custom Cellpose training (on by default). Clip
to the image's own range (np.iinfo max for integer dtypes, 1.0 for float), keep
the augmented variants the same dtype as the originals, and scale the noise std to
the range. Keeps training data in the same uint8 [0,255] space prepare_cellpose
feeds the model downstream (cellpose normalizes internally at train + inference).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@mat10d @claude
fix(segment): use the h-maxima transform for peak suppression, not h_…
014641d 
…minima

apply_h_minima_suppression ran skimage h_minima on the distance/intensity peak
map, marking valleys instead of peaks — so with maxima_reduction_factor set, the
watershed seeds landed in background. Replace with the grayscale h-maxima transform
(reconstruction of peak_map - h under peak_map), which flattens maxima shallower
than h while keeping the map grayscale for peak_local_max. Rename the function +
docstrings to h-maxima to match.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@mat10d @claude
fix(aggregate): don't abort when a well has zero secondary objects
18b1b6d 
aggregate_second_objs validated merge keys on second_objs_df before use, so a
well with no detected secondary objects (empty frame, no keys) raised
'Missing merge keys' and killed the whole aggregate. Short-circuit the empty
case: keep all cells unchanged (as the 'none' strategy does for that well); they
concat cleanly with object-bearing wells.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@mat10d @claude
fix(aggregate): align second-object merge-key dtypes to avoid silent …
1b2fecd 
…all-NaN

cells (from final_merge) and second_objs (from the phenotype parquet) can carry
the plate/well/tile/cell keys at different dtypes (e.g. int32 vs int64, int vs
float). A how='left' merge on mismatched dtypes matches nothing and silently
fills every second-object feature with NaN. Coerce the second-object keys to the
cells' dtypes before merging so matches land.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@mat10d @claude
fix(phenotype): default second_obj_detection to False (opt-in)
ddd5ed6 
The secondary-object pipeline gated on config['phenotype'].get('second_obj_detection',
True), so any screen that never set the key silently ran the whole feature and
picked up its phenotype/aggregate schema + path changes. Default to False so
secondary objects are strictly opt-in; existing screens get the standard pipeline
unchanged. Screens using the feature set the flag explicitly (as the small_test
config does).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@mat10d @claude
feat(sbs): emit per-cycle alignment-offset metrics + broadcast to sbs…
fc856ec 
…_info (symmetric with phenotype)

Mirror the phenotype alignment-metrics pattern on the SBS side:
- add shared offsets_to_metrics(offsets, unit) helper in lib/shared/align.py
- align_cycles() gains return_metrics kwarg; captures per-cycle offsets
  from whichever alignment branch runs (DAPI or sbs_mean)
- scripts/sbs/align_cycles.py writes a one-row alignment_metrics TSV
- align_sbs rule gets a 2nd output (metrics TSV); extract_sbs_info consumes
  it as input[1] so the shared extract_phenotype_minimal broadcasts the
  offset columns onto every cell row
- register the metrics TSV target in targets/sbs.smk
- mark offset_* columns as metadata in cell_data_metadata_cols.tsv

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@mat10d @claude
feat(aggregate): make compartment_combo optional via split_by_compart…
e0b125e 
…ment toggle

The secondary-object feature stamped a compartment_combo dimension onto every
aggregate/cluster/bootstrap output path (_CmCo- token, /{combo}/ cluster dir,
__{combo}__ bootstrap segment) and required the column in the combo TSVs. This
broke backward compatibility for screens that don't use secondary objects:
missing-column KeyErrors and a forced full re-run from changed paths.

Add config aggregate.split_by_compartment (default False). When off, paths are
byte-identical to pre-feature output and a missing compartment_combo column is
tolerated; when on, compartment-specific paths are emitted as before. Column
filtering is already a no-op with all compartments present, so default mode is
a pure path/plumbing change with no effect on the data.

- new lib/shared/compartment_utils.py owns the conditional token/dir/suffix
  emission, the wildcard accessor, and combo-table/record normalization
- Snakefile initializes the toggle + all-compartments default and normalizes
  the aggregate/cluster combo tables at read time
- aggregate/bootstrap/cluster targets and rules route paths through the helpers
  and resolve the combo via the accessor instead of a required wildcard
- metrics/cluster eval resolve legacy or compartment paths and gate the
  compartment label in stats keys/columns
- small_test config sets split_by_compartment: true so the fixture keeps
  exercising the opt-in feature

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@mat10d mat10d mentioned this pull request Aug 4, 2026
Merged
mat10d and others added 8 commits August 4, 2026 16:04
@mat10d @claude
fix(aggregate): treat offset_* alignment QC as metadata by prefix
00c1b85 
The per-cell alignment-offset columns (offset_y/x, offset_*_stepN, offset_*_cycleN)
broadcast onto phenotype_cp / sbs_info are QC diagnostics, not clustering features. Their
names depend on the screen's alignment step/cycle count, so enumerating them in the
metadata_cols file is screen-specific and easy to under-specify. Any offset_* column not
listed silently falls through into the feature space (split_cell_data, filter,
perturbation_score), polluting aggregation/clustering.

Reserve the offset_ prefix as metadata via a shared is_reserved_metadata_col predicate,
applied wherever metadata is split from features. No-op where offsets are already
enumerated (e.g. the small_test config); closes the silent-leak path for real screens
whose cycle/step count differs from the list.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@mat10d @claude
test: drop enumerated offset_* from metadata cols (now prefix-recogni…
b070125 
…zed)

The offset_* alignment-QC columns were hand-enumerated in the small_test metadata_cols
file back when enumeration was the only way to keep them out of the feature space. The
offset_ prefix rule makes that unnecessary, so remove the 32 offset_* lines: the fixture
now models correct usage (screens shouldn't enumerate offsets) and actually exercises the
prefix path. Also drop a stale num_nuclei mention in a comment.

Verified: full small_test_analysis aggregate re-run 1221/1221, aggregated features carry 0
offset_ columns, per-cell tables still retain offset_* as metadata via the prefix.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@mat10d @claude
refactor(phenotype): tidy secondary-object segmentation code
b23a896 
Cleanup pass over the secondary-object feature code:

- Move the ML/classical parameter routing out of
  scripts/phenotype/identify_second_objs.py into a lib helper
  (segment_second_objs_from_config); the script drops from 161 to 59 lines and the
  unused nuclei_id_col scaffolding is removed.
- segment_second_objs and create_empty_results now always return a 3-tuple (updated
  cytoplasm masks are None when no cytoplasm masks are given), so callers that unpack
  three values can't hit a shape mismatch; two dead identical if/else return branches
  are collapsed.
- Narrow a bare `except` in extract_phenotype_second_objs to IndexError, so only a
  sub-3D array falls back to channel 0 and real errors surface.
- After declumping, fill_holes only fills background pixels, so a filled hole cannot
  overwrite a neighboring object's pixels.
- cellpose_training uses a local np.random.Generator instead of seeding the global RNG,
  makes augmentation seedable/reproducible, and keeps at least one training sample when
  only a single sample is available.
- _aggregate_single preserves the input cell order across its split/merge.

Verified with py_compile, ruff, and focused in-memory checks. The small_test gate runs
second_obj_detection=false, so it does not exercise this path.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
acepedadiaz @claude
fix(metrics): repoint DEFAULT_METADATA_COLS at its defining module
9398480 
f43a39d removed `from lib.phenotype.constants import DEFAULT_METADATA_COLS`
from cell_data_utils.py as an unused import. It was a re-export: metrics.py
imported the constant *through* cell_data_utils, so both call sites now raise
ImportError.

Both are function-local imports, so py_compile and ruff do not see them --
get_all_stats() calls get_phenotype_stats() unconditionally and fails at
runtime, and _calculate_batch_effects() fails under include_batch_effects=True.

Import from lib.phenotype.constants directly rather than restoring the
re-export. constants.py has no imports of its own, so there is no cycle.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
acepedadiaz @claude
fix(aggregate): guard empty secondary objects at screen level, not pe…
87ccb6a 
…r well

18b1b6d returned cells_df unchanged when a well had no secondary objects.
That sits after the "none" short-circuit, so it only fires when the user
explicitly asked for single/all/average, and it conflated two cases.

A per-well empty slice is legal: second_objs_filtered is a boolean slice of
the full parquet, so it keeps its columns, and _aggregate_single NaN-fills
every cell -- the documented contract. Returning early instead emits a well
whose parquet lacks those columns, and harmonize_pool_schema keeps only the
intersection across wells, so one empty well silently strips the secondary
object features from every well. A screen with a class-pure control well
could lose them entirely and still complete.

The case that should fail is the whole screen having no secondary objects
while a non-"none" strategy is set. Check that in the script, where the
unfiltered frame is still in scope, and drop the per-well early return so
empty wells NaN-fill as documented.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
acepedadiaz @claude
docs(phenotype,aggregate): finish the Google-style conversion, fix st…
69f86e7 
…ale returns

9d65852 converted file_utils.py, cellpose_training.py and second_obj_utils.py
but left segment_secondary_object.py -- its only change there was relocating two
functions. Convert the 12 remaining NumPy-style docstrings in that file.

Ruff does not catch this: convention = "google" checks header punctuation, not
body layout, which is why several docstrings carried a "Returns:" colon with
NumPy dashes still underneath.

Also correct descriptions the earlier commits invalidated:

- segment_second_objs, segment_second_objs_ml and create_empty_results return an
  unconditional 3-tuple whose third element is None when no cytoplasm masks are
  given. Four Returns entries still said "if provided" / "optionally", which
  reads as a 2-tuple. Match _postprocess_secondary_objects, already correct.
- harmonize_pool_schema documented kept_metadata_cols as a subset of the caller's
  metadata_cols, but 00c1b85 appends reserved offset_* columns that were never in
  that list.

Docstrings only: ASTs with docstrings stripped are identical to b23a896 for both
files. ruff check and ruff format clean.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
acepedadiaz @claude
Merge branch 'main' into secondary_object
96abf7f 
Bring main (5 commits, #227/#231/#232/#234/#240) into the branch ahead of
#171. Merged rather than rebased: a rebase replays 45 commits and re-hits the
same aggregate collisions repeatedly, while the merge conflicts in one file,
and main only accepts squash merges so the linear history would be flattened
on landing anyway.

Both conflicts are in generate_feature_table.py and are complementary:

- #234 added a warning when perturbation_id_col is unset; the branch added the
  control_name_col fallback (#222). Independent, both kept.
- Column ordering for the construct table: the branch dedupes via
  dict.fromkeys over [pert_id_col, pert_col, control_name_col, ...], main
  dedupes only the id/name pair. Kept the branch version -- main's drops
  control_name_col, which the control filter downstream indexes on, so main's
  form would KeyError under a construct-level config.

ruff check and ruff format clean.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@mat10d @claude
ci: re-trigger checks on #171
fd65ef7 
Empty commit to fire a fresh pull_request event so the Test Analysis and Ruff
workflows run against the current secondary_object HEAD (they last ran on the
pre-merge head; the recent updates did not create runs).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
mat10d
mat10d approved these changes Aug 7, 2026
View reviewed changes

@mat10d mat10d left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small comments, @mat10d to address.

Comment thread workflow/rules/aggregate.smk Outdated
SECOND_OBJ_DETECTION = config["phenotype"].get("second_obj_detection", False)


def get_rule_compartment_combo(wildcards):

@mat10d mat10d Aug 7, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strip functions from .smk if possible to observe conventions

Comment thread workflow/scripts/aggregate/aggregate.py

# Carry the control/gene name column through when it differs from the aggregation ID.
pert_col = snakemake.params.perturbation_name_col
control_name_col = snakemake.params.get("control_name_col")

@mat10d mat10d Aug 7, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirm this is passed through/flag for aggregate notebooks

Comment thread workflow/scripts/aggregate/aggregate_cells_second_objs.py Outdated
print(f" Secondary objects: {len(second_objs_df)} rows")

# Screen-level guard: asking to integrate secondary objects into a screen that has
# none is a misconfiguration, not a data condition. Checked here (before the

@mat10d mat10d Aug 7, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove multiline comments

@mat10d @claude
style(aggregate): address review — .smk helpers to lib, single-line c…
db073ba 
…omments

- Move format_rule_output into lib/shared/compartment_utils.py and bind the two
  compartment-config helpers with functools.partial in rules/aggregate.smk, so the rule
  file no longer defines Python functions (logic lives in lib; call sites unchanged).
- Collapse the multi-line "#" comment blocks this branch introduced to single lines
  (second_obj_utils, segment_secondary_object x2, aggregate_cells_second_objs,
  generate_feature_table), per house style.

Verified: py_compile + ruff clean; snakemake dry-run builds the aggregate DAG (the rule
input functions resolve through the partials).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@mat10d

mat10d commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator

Review notes addressed (db073ba)

  • .smk helpers → moved format_rule_output into lib/shared/compartment_utils.py and bound the two compartment helpers with functools.partial in rules/aggregate.smk, so the rule file no longer defines functions (call sites unchanged). A scan confirmed these were the only two defs in any .smk.
  • Multi-line comments → collapsed the # blocks this branch introduced to single lines (second_obj_utils, segment_secondary_object ×2, aggregate_cells_second_objs, generate_feature_table). Pre-existing multi-line blocks elsewhere were left alone to keep the diff scoped.
  • control_name_col → confirmed fully wired (see inventory below); it's optional and backward-compatible.

New config parameters this PR wires (for the analysis notebooks)

Complete set of config knobs this PR threads end-to-end. All are read from config.yml; defaults keep existing screens unaffected. This is the surface the brieflow-analysis notebooks should expose.

phenotype

  • second_obj_detection — bool, default false. Master switch; gates every new phenotype/aggregate rule.
  • second_obj_channel_index — int, required when detection is on. Channel holding the secondary objects.
  • Shared (optional, default): second_obj_min_size (10), second_obj_max_size (200), size_filter_method ("feret"), max_objects_per_cell (120), overlap_threshold (0.1), max_total_objects (1000), use_ml_segmentation (false).
  • Classical path (optional): threshold_smoothing_scale (1.3488), threshold_method ("otsu_two_peak"), use_morphological_opening (true), opening_disk_radius (1), fill_holes ("both"), declump_method ("shape"), declump_mode ("watershed"), suppress_local_maxima (20), maxima_reduction_factor (None), use_shape_refinement (false), proportion_threshold (0.4).
  • ML path: logscale (true) + any extra phenotype keys pass through to the ML segmenter.

aggregate

  • split_by_compartment — bool, default false. When true, aggregate/cluster outputs split per compartment_combo; when false, output paths are byte-identical to pre-feature.
  • second_obj_agg_strategy — none | single | all | average, default none. How per-object features fold up into cell rows.
  • generate_montages — bool, default true. Toggle montage QC outputs.
  • bootstrap_combinations[].compartment_combo — optional per-entry compartment target.

Combo TSVs

  • New compartment_combo column in aggregate_combo.tsv / cluster_combo.tsv. Defaults to all-compartments when the column is absent or split_by_compartment is off.

Already on main, newly consumed here (document the usage, not new knobs)

  • aggregate.control_name_col — optional, falls back to the perturbation column; now also read by generate_feature_table / aggregate.
  • aggregate.drop_cols_threshold — optional; now also read by generate_feature_table / align.

The remaining follow-up is analysis-side: surface these in the aggregate/phenotype notebooks so operators can set them.

@mat10d @claude
style(aggregate): explicit lib calls in rules instead of functools.pa…
f50e550 
…rtial

Match the convention used across the other rule files (inline
`lambda wildcards: lib_fn(wildcards, config...)`): drop the two functools.partial
bindings and call get_compartment_combo / format_rule_output directly with
SPLIT_BY_COMPARTMENT + DEFAULT_COMPARTMENT_COMBO passed explicitly. No functions are
defined in the .smk; the helpers live in lib/shared/compartment_utils.py.

Verified: snakemake dry-run builds the aggregate DAG.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mat10d mat10d mat10d approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@akcd1 @mat10d @EdenYifrach