refactor update_all_cells

[drbergman]

This is responsive to issue #320

• fix bug in neighbor list
  • phagocytosis and fusion could result in removal of a cell
  • but the neighbor list was not updated
• resolve interactions after phenotype updates
  • this allows the update velocity step to update the neighbors list one time
  • does move interactions before several steps:
    • gradient computation
    • evaluate interactions (so fewer interactions to evaluate)
    • custom rule
    • update velocity
    • springs
• do not allow spontaneous spring detachments caused by attack (now a separate pr in disallow random detachment from attack #340 )
• refactor how timing is done for simplicity

[heberlr]

We need to discuss this change in call order further, as it will impact phagocytosis, attack, and fusion. I noticed it also relates to other PRs (#340 and #341). This change might introduce too many side effects.

[drbergman]

Yes, very happy to discuss more. By side effects, do you mean unintended behavior and/or errors? Or do you mean changes from how it ran before?

If we find this to be a long discussion, I would prefer to get a quick fix for this bug while we keep working on a longterm fix. Perhaps something like (the highly inefficient) #339?

[heberlr]

I mean unintended behavior (like in PR #340 ), as the attach and detach dynamics were working fine in the previous release. I feel that by making this call order change, we might introduce these issues and possibly others we haven’t considered yet. Does that make sense?

[drbergman]

Yeah, that makes sense. To be clear, #340 and #341 are built off the dev branch / previous release (1.14.0), not this fix. Their point of origin is how we implemented changes in 1.14.0. This fix does not change the existence of those behaviors.

[MathCancer]

I don't think the correct fix to a bug is to reorder operations to avoid the bug.

[drbergman]

Another option that @rheiland worked out was to update the delete_cell() function to have it also update neighbors the same way it already handles updating spring attachments

[MathCancer]

I do think we need to say: if lingering neighbors after cell deletion is the problem, then the correct fix is to correct those neighbor lists at the time of cell deletion.

(This is what we did in spring links. We just do the same for neighbor lists now.)

If A is neighbor to (B,C,D), then B, C and D are also neighbors to A. When A is deleted, remove A from the neighbor lists of B, D and D

[drbergman]

Here was where @rheiland documented this fix: #320 (comment)

My main reason for not opting for this solution is that we do not enforce that the neighbors is a symmetric relationship. The implementation of standard_update_cell_velocity produces a symmetric relationship (by its reliance on add_potentials), but we do not impose this restriction on user-defined update_velocity functions

[drbergman]

another fix is being formulated...