fix: PhysiCell reader orientation and label handling - #2
Conversation
955acbf to
353d27f
Compare
Resolve cell matrix orientation from the frame's declared variable count instead of comparing matrix dimensions. The old heuristic assumed every frame holds more cells than the model has variables, silently returning transposed data for sparse initial conditions, early time steps, and simulations approaching extinction. Derive column positions from the MultiCellDS <labels> block, which was already parsed into metadata.extra['custom_labels'] but never read. This makes custom and non-indexed columns reachable via a new 'columns' key, and lets models with differing variable counts parse in one session. Also fix to_dataframe() ignoring include_dead_cells, add an orientation guard to parse_microenvironment_mat, return raw_data as loaded from disk, and single-source __version__ from distribution metadata. Reported by the llm-abm-consistency harness (OHSU ChangLab).
353d27f to
7cd038f
Compare
|
Rebased onto Conflict resolution. Eight regions across Also removed Verified on the rebased branch:
Unchanged from the original review: issue #3 from the report (wiring |
Fixes issues #1, #2, #4, #5, and #6 from the v0.2.1 defect report filed by the llm-abm-consistency harness (OHSU ChangLab, 2026-07-21). All six findings were reproduced before fixing; four of them reproduce against this repo's own bundled example simulation, not just the reporters' dataset.
Issue #3 (wiring
parse_microenvironment_matinto the reader classes) is deliberately not in this PR — see below.Why these shipped undetected
The bundled example simulation writes 154 variables per frame, and its smallest frame across all 696 timesteps holds 906 cells.
n_cells > n_variablesholds everywhere, so the orientation heuristic never fires and all 40 existing PhysiCell tests pass while the defect sits there. This is a fixture-coverage gap as much as a code defect, which is why the new tests build matrices where that assumption fails.Changes
Issue #2 — orientation (High). Orientation now resolves by matching the shape against the declared variable count. A matrix matching neither orientation raises
ValueErrorrather than returning data. With no labels available, the legacy heuristic still applies but emits aUserWarningnaming the ambiguity.raw_datais now returned exactly asloadmatgave it, with a new'orientation'key describing its layout — previously it was the reoriented matrix, so callers couldn't recover the truth when the heuristic misfired.Reproduced before the fix, truncating a real frame to 40 cells:
2494.0is the PhysiCell default cell volume — the transposed read was serving the volume column as an x-coordinate.Issue #1 — labels (High).
parse_cells_matgains alabels=parameter and derives column positions from the frame's own<labels>block. Precedence isindex_mapping>labels> row-count autodetect. A new'columns'key exposes every labelled column. The reader passes labels through automatically, so each frame resolves against its own XML.Issue #4 — dead cells (Medium).
to_dataframe()now honorsinclude_dead_cells. On a real frame it returned 985 rows whilen_cells,positions, andto_spatial_data()all reported 922.Issue #6 — microenvironment guard (Low). Validates
shape[0] == 4 + len(substrate_names), transposing or raising otherwise.Issue #5 — version (Low).
__version__now reads from distribution metadata. It reported0.1.0at thev0.2.0tag. Note this was three sources of truth, not two —conf.pyandpyproject.tomlwere both updated during v0.2.0 prep and__init__.pywas missed, which is exactly the drift the reporters predicted.Two corrections to the spec
max(labels) + 1undercounts. The spec proposes this for the expected variable count, but it ignores the width of the final label — a trailingsize == 3vector undercounts by two.declared_variable_count()usesmax(index + size)instead.Orientation is not derived from
index_mapping. The spec proposesmax(index_mapping.values()) + 1as an alternative source. Index mappings are sparse:CELL_DATA_INDICES_V2tops out at 39, but real frames here have 154 rows, so this would reject every valid matrix. Orientation comes fromlabelsonly; explicitindex_mappingstill controls column positions.Deviation on scope
to_dataframe()emitting every labelled column is behindextra_columns=Truerather than on by default. A full frame here carries 154 variables, andto_trajectory_dataframe()concatenates across all timesteps — the docstring already warns about memory, and a 14× column increase by default seemed wrong for a patch release. The columns are always available viaparse_cells_mat(...)['columns'].I also took the straight fix on #4 rather than the opt-in alternative the spec offers.
to_dataframe()disagreeing withpositionson both row count and row order is a bug, not an interface.include_dead_cells=is still accepted per-call to recover the old output.Backward compatibility
Additive except for two deliberate changes, both in the changelog:
raw_datais no longer transposed, andto_dataframe()filters dead cells by default. Withlabels=None, parser output is unchanged from v0.2.0 — verified field-by-field against a real frame.Testing
25 new tests in
tests/test_physicell_labels.pycovering every acceptance criterion in the report, including all four orientation cases, cross-model label isolation (87 vs 91 variables in one session), and the version assertion. Full suite: 481 passed, 2 skipped (the 2 skips are pre-existing, optional network deps).Fixtures are generated in-test with
savematrather than committed as binaries.Not included
Issue #3 — microenvironment wiring (
substrates,voxel_positions,substrate_at, and thediscover_physicell_timestepssignature change) — is an API surface addition, not a bug fix, and the dataclass return type is a breaking change worth doing deliberately. Tracked for v0.3.0 and noted under "Known gaps" in the changelog. The reporters offered to upstream their KDTree implementation; worth taking them up on it, and re-verifying theargminequivalence claim here rather than on their sample of 300 query points — ties on a regular voxel grid are exactly where that sample can miss.