Give each cell definition its own fixed_duration and death-phase parameters - #424
Open
drbergman wants to merge 3 commits into
Open
Conversation
…meters Fixes MathCancer#199. Cycle_Model objects are shared by pointer between cell definitions: apoptosis and necrosis are globals, and every definition registers the same address via add_death_model( rate, &apoptosis, ... ). Cell_Definition's copy constructor copies the pointer, not the model. fixed_duration lived in Phase_Link, inside that shared object, while the XML parser wrote it per cell definition. So each definition overwrote the flag for every other one and the last definition parsed decided for all of them. A model asking for deterministic apoptosis on most of its cell types silently got the stochastic branch on all of them, with no warning and nothing in the saved output to reveal it. Migrate the flag from the graph structure to the param structure, as Paul suggested on the issue: Cycle_Data gains fixed_durations, indexed exactly like transition_rates, with fixed_duration() / exit_fixed_duration() accessors mirroring transition_rate() / exit_rate(). Cycle_Model::advance_model now reads the per-cell copy. Phase_Link::fixed_duration is removed; the six standard models declare their defaults through the model's own Cycle_Data instead, and those defaults still reach cells that do not override them in XML. That alone is not enough for the death models. There is no per-definition Cycle_Data for a death model -- Death holds rates, models and parameters, none of which carry cycle parameters -- so the parser wrote durations to the shared models[i]->data, and Cycle::sync_to_cycle_model() copies that same shared data over the cell at start_death(). Relocating the flag would have moved it from one shared object to another. This is also why rheiland observed on the issue that the <duration> value is flattened along with the flag. So Death gains model_data, one Cycle_Data per death model, seeded from the model at add_death_model(), written by the parser, and applied at start_death() through a new two-argument Cycle::sync_to_cycle_model( cm, cd ). The single-argument form delegates to it and keeps its exact previous behaviour. Verified on a 7-cell-type model where six definitions ask for <phase_durations fixed_duration="true"> and the last asks for a rate. Before: all seven report fixed=0. After: six report 1 and the last reports 0, while all seven still share one Cycle_Model object, so the phase graph stays shared and Phenotype copies stay cheap. A definition with no apoptosis <phase_durations> at all still inherits the standard model's default. Note: this changes results for any model that mixes <phase_durations> and <phase_transition_rates> across its cell definitions. Those models were not getting the death timing their XML asked for; they will now. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes a long-standing bug (#199) where fixed_duration (and related phase timing parameters) were effectively shared across cell definitions due to shared Cycle_Model instances—most notably for the standard apoptosis/necrosis death models—causing later-parsed XML definitions to overwrite earlier ones.
Changes:
- Moves the
fixed_durationflag fromPhase_Link(shared graph) intoCycle_Data(per-instance parameters) and updates standard model defaults accordingly. - Introduces per-definition death-cycle parameter storage via
Death::model_data, and applies it at death start using a newCycle::sync_to_cycle_model(cm, cd)overload. - Updates XML parsing and death start logic to write/apply per-definition (non-shared) cycle/death timing parameters.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| core/PhysiCell_standard_models.cpp | Updates standard models to set fixed-duration defaults via Cycle_Data rather than Phase_Link. |
| core/PhysiCell_phenotype.h | Removes Phase_Link::fixed_duration, adds Cycle_Data::fixed_durations, adds Death::model_data, and adds a 2-arg Cycle::sync_to_cycle_model. |
| core/PhysiCell_phenotype.cpp | Implements fixed_duration accessors, switches cycle advancement to read per-cell Cycle_Data, seeds Death::model_data, and adds the 2-arg sync implementation. |
| core/PhysiCell_cell.cpp | Applies per-definition death-cycle parameters at start_death() and updates XML parsing to write into per-definition data instead of shared models. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Cell::advance_bundled_phenotype_functions syncs the cycle to the death model directly when check_for_death fires, rather than going through start_death. That site was still using the single-argument sync_to_cycle_model, so cells dying by death_rate -- the common case -- kept getting parameters from the shared Cycle_Model and MathCancer#199 was unfixed on that path. Also from review: hoist the transition_rates / fixed_durations resize out of the inner phase-link loop in Cycle_Data::sync_to_cycle_model, and take the Cycle_Data by const reference in the new Cycle overload. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
display_cell_definitions() read the death phase durations out of death.models[k]->data, the shared Cycle_Model. The parser no longer writes that object, so after the rest of this PR the summary printed the compiled-in standard-model defaults for every definition regardless of the XML -- and that summary is exactly what a user reads to check whether MathCancer#199 is fixed. Take the parameters from death.model_data[k] instead. Phases and phase links still come from the shared model, which is correct: only the parameters moved. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
drbergman
marked this pull request as ready for review
August 8, 2026 19:57
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #199.
Death::modelsis astd::vector<Cycle_Model*>holding the addresses of theapoptosisandnecrosisglobals, so every cell definition shares one death model object.fixed_durationlived inPhase_Link, inside that shared object, and the parser wrote it per cell definition — so each definition overwrote the flag for every other one and the last one parsed decided for all of them. The<duration>value went the same way, since the parser also wrotemodels[i]->transition_rate(...).This is specific to death. The live cycle was never affected:
Cell_Functions::cycle_modelis aCycle_Modelheld by value, so each definition owns its own copy oflive,Ki67_basicand the rest, and itsPhase_Linkflags were already per-definition.What this does
Migrates the flag from the graph structure to the param structure, which is what @MathCancer proposed on the issue.
Cycle_Datagainsfixed_durations, indexed exactly liketransition_rates, withfixed_duration(i,j)andexit_fixed_duration(i)mirroringtransition_rate(i,j)andexit_rate(i).Phase_Link::fixed_durationis removed, and the six standard models declare their defaults through the model's ownCycle_Data. For the live cycle this is plumbing rather than a fix — it gives death models somewhere per-definition to keep the flag. Its one behavioural effect is that the flag is now per-cell rather than per-definition, which is howtransition_ratesalready worked.Cycle_Datafor a death model, andCycle::sync_to_cycle_model()doesdata = cm.data, so even a correctly per-definition flag would be overwritten from the shared model at the moment of death — which is also why the<duration>value is flattened along with the flag, as @rheiland observed.Deathgainsmodel_data, oneCycle_Dataper death model, seeded atadd_death_model(), written by the parser, applied through a new two-argumentsync_to_cycle_model( cm, cd ).start_death()and thecheck_for_death()branch ofadvance_bundled_phenotype_functions(). The second matters most, since that is the path a cell takes when it dies from itsdeath_rate.display_cell_definitions()reports the per-definition parameters rather than the shared model's.Verification
interaction-sample, withstemasking for a fixed 100 min apoptosis,differentiatedfor 700, andneutrophil— parsed last, so previously the one that won — for a rate. Logging every cell as it enters a death model, over 649 deaths and 61 transformations:stemdifferentiateddifferentiated, having transformed fromstemneutrophilbacteria, no<phase_durations>in XMLA transformed cell picks up the death parameters of the definition it became:
convert_to_cell_definition()doesphenotype = cd.phenotype, somodel_datatravels withmodelsandrates. The two stayed the same length at all 649 deaths. The startup summary now prints the same per-definition values.Compatibility
Results change for any model whose cell definitions do not all give their death models the same parameters. Two things were being flattened onto the shared model, independently of each other:
Whichever cell definition was parsed last set both, for every definition.
Neither is tied to how the XML expresses it.
<phase_durations>and<phase_transition_rates>write the same rate slot —exit_rate(i)istransition_rates[i][0], andtransition_rate(i,j)resolves there too for a single-link death phase — and both readfixed_durationfrom the attribute with identical logic, so a definition can perfectly well ask for a duration with a stochastic exit or a rate with a deterministic one.Of the two, the boolean is the more disruptive to get wrong: it switches a phase between a deterministic exit and a Poisson process with the same mean, so aggregate numbers look plausible while the variance is entirely different and the phase can now be exited on its first step. The rate being flattened is a plain quantitative error.
Live cycle timing is unchanged.
Phase_Link::fixed_durationis removed, so out-of-tree code settingphase_link(i,j).fixed_durationneedsdata.fixed_duration(i,j)instead. Nothing in this repository outsidecore/uses it in C++.