Conversation
|
|
||
| for (int i = 0; i < objs.numObjs; i++) { | ||
| Object o = objs.objs[i]; | ||
| if (!(o instanceof arcade.potts.agent.cell.PottsCell)) { |
There was a problem hiding this comment.
To clarify, is this if statement for avoiding environmental objects (like vasculature)? Otherwise, we can't have a simulation where Patch/Potts cells both exist right?
| * | ||
| * @param sim the simulation | ||
| */ | ||
| public void updateGrowthRate(Simulation sim) { |
There was a problem hiding this comment.
Very nitpick comments so feel free to ignore:
-
Consider factoring out the volume averaging logic to its own function
-
For readability it might be easier to do switch statements or if/else statements where the boolean is true. For example something like:
default:
cellGrowthRate = cellGrowthRateBase
case pdelike
cellGrowthRate = averageVolume()
case dynamicGrowthVolume
cellGrowthRate = updateCellVolumeBasedGrowthRate()
| <population.module module="proliferation" id="NUCLEUS_CONDENSATION_FRACTION" value="0.5" description="fraction of nuclear volume when condensed" /> | ||
|
|
||
| <!-- ── Fly cell proliferation module parameters ───────────────────── --> | ||
| <!-- ── Dictating whether NB behavior should be the same across all NBs ──────── --> |
There was a problem hiding this comment.
Another very nitpick comment so feel free to ignore
- Currently the comments feel more like parameter descriptions rather than categories -- consider organizing the proliferation parameters by model type?
Eg:
<!-- ── Dynamic volume model parameters ──────── -->
...
<!-- ── Constant volume parameters ──────── -->
...
<!-- ── PDE like model parameters ──────── -->
...
There was a problem hiding this comment.
I agree that the <!-- ── Dictating whether NB behavior should be the same across all NBs ──────── --> and <!-- ── Growth rate sensitivity to cell volume dynamics ───────────────────── --> read more like descriptions than categories. I think part of it is because all the other labels end in "parameters"
I'm wondering why we need to further categorize PDELIKE, DYNAMIC_GROWTH_RATE_VOLUME, and GROWTH_RATE_VOLUME_SENSITIVITY... would it be okay to just remove the sublabels?
| count++; | ||
| } | ||
| } | ||
| double avgVolume = volSum / count; |
There was a problem hiding this comment.
I think Allison touched on this in an earlier comment, but I think that finding the average volume of the Potts cells can be moved out into their own function like getAvgVolume(Simulation sim) or something.
| <population.module module="proliferation" id="NUCLEUS_CONDENSATION_FRACTION" value="0.5" description="fraction of nuclear volume when condensed" /> | ||
|
|
||
| <!-- ── Fly cell proliferation module parameters ───────────────────── --> | ||
| <!-- ── Dictating whether NB behavior should be the same across all NBs ──────── --> |
There was a problem hiding this comment.
I agree that the <!-- ── Dictating whether NB behavior should be the same across all NBs ──────── --> and <!-- ── Growth rate sensitivity to cell volume dynamics ───────────────────── --> read more like descriptions than categories. I think part of it is because all the other labels end in "parameters"
I'm wondering why we need to further categorize PDELIKE, DYNAMIC_GROWTH_RATE_VOLUME, and GROWTH_RATE_VOLUME_SENSITIVITY... would it be okay to just remove the sublabels?
| when(gmcB.getCriticalVolume()).thenReturn(100.0); | ||
| when(gmcC.getCriticalVolume()).thenReturn(200.0); | ||
|
|
||
| // Noise: different type and/or different pop → must be ignored |
There was a problem hiding this comment.
If you end up moving the average volume calculation into a separate function, then I think parts of this test can also be moved to test just the average volume calculation...
| * | ||
| * @param sim the simulation | ||
| */ | ||
| public abstract void updateGrowthRate(Simulation sim); |
There was a problem hiding this comment.
Could the updateGrowthRate method look different for each type of volume-based proliferation?
I am a little confused as to why the updateGrowthRate logic is a method rather than some switch statements in the abstract class
| <!-- ── Fly cell proliferation module parameters ───────────────────── --> | ||
| <!-- ── Dictating whether NB behavior should be the same across all NBs ──────── --> | ||
| <population.module module="proliferation" id="PDELIKE" value="0" description="1 → all cells of a given type have same growth and division dynamics based on average metrics across the population; 0 → Cell growth and division is determined on a cell-by-cell basis, cells can differ from other cells of the same type" /> | ||
| <!-- ── Growth rate sensitivity to cell volume dynamics ───────────────────── --> |
There was a problem hiding this comment.
I recall that there were non-volume based proliferation options, do you intend to have a section for that too?
After some reading I do think it would make sense to have something like:
<---- fly stem cell proliferation module params ----->
<---- booleans that modulate proliferation growth rate ----->
<------ volume based proliferation parameters ----->
<----- time based proliferation parameters ----->
<----- (other types of fly cell proliferation ----->
| when(parameters.getDouble("proliferation/SIZE_TARGET")).thenReturn(1.2); | ||
| when(parameters.getInt("proliferation/PDELIKE")).thenReturn(0); | ||
|
|
||
| // critVol = 150.0; sizeTarget = 1.2 |
There was a problem hiding this comment.
is this for documentation or leftover comments?
If its for documentation its not immediately clear what these values are for
| } | ||
|
|
||
| @Test | ||
| public void computeEquilibriumVolume_differentSizeTarget_scalesCorrectly() { |
There was a problem hiding this comment.
very nitpick but for documentation might be clearer to include the multiplier in the title
e.g. computeEquilibriumVolume_doubleSizeTarget_scalesCorrectly()
| /** | ||
| * Sensitivity of growth rate to cell volume, only relevant if dynamicGrowthRateVolume is true. | ||
| */ | ||
| final double growthRateVolumeSensitivity; |
There was a problem hiding this comment.
Ignore if this was implemented elsewhere (or if its impossible to make this error), but might be good to throw an error if growthRateVolumeSensitivity is set but dynamicGrowthRateVolume is false. If you do implement this error throw it would be good to add a test for it as well
|
Finished my first pass of reviews - overall looks good but I have a couple of discussion points/questions to be resolved before approving |
Estimated time to review: Medium
Summary of changes:
In general this PR adds volume-sensitive growth and PDE-like growth for the fly GMC cells (which the Neuroblasts will also use)
How to verify changes:
Here is a minimal GMC setup file:
You can add/adjust the following parameters and run simulations:
Dynamic growth rate volume is a boolean you can turn on and off. When on (1) small cells grow slower than big cells.
Growth rate volume sensitivity is the exponent that determines the slope of the sigmoid (essentially the hill coefficient) dictating how sensitive the growth rate is to the cell's volume.
PDELIKE is a boolean that, when on, makes the cell growth rate a function of the average GMC volume across all GMCs in the simulation.