rmnldwg / lymph

Python package for statistical modelling of lymphatic metastatic spread in head & neck cancer.
https://lymph-model.readthedocs.io
MIT License
5 stars 4 forks source link

matrices not updated in bilateral model #68

Closed YoelPH closed 5 months ago

YoelPH commented 5 months ago

While debugging my code I stumbled over an interesting problem which only arises when changing the modalities of a bilateral model.

Updating the modalities in any model should update the observation_matrix. This also works for the ipsilateral side, but not for the contralateral side. It seems as if callback for the contralateral side is not successful. I am not 100% sure whether this is due to the synchronization or if there is another problem.

I am working on a fix, but I wanted to notify you since you probably can detect the problem faster and produce a better fix.

Edit: my fast fix was quite simple. Since the problem is that the contralateral side does not trigger callbacks anymore, I am applying a different syncing function for the modalities:

def init_dict_sync2(
    this: AbstractLookupDict,
    other: AbstractLookupDict,
) -> None:
    """Add callback to ``this`` to sync with ``other``."""
    def sync():
        other.clear()
        other.update(this)

    this.trigger_callbacks.append(sync)

Thus we also need to change the way modalities are synced in the init_synchronization function:

       # Sync modalities
        if self.is_symmetric["modalities"]:
            init_dict_sync2(
                this=self.ipsi.modalities,
                other=self.contra.modalities,
            )

I am sure that you can come up with a more elegant solution, as I am struggling a bit to get a good overview of all the callbacks and syncing functions.

rmnldwg commented 5 months ago

I noticed the same issue occurs for the transition matrix.

Doesn't your solution create an infinite loop? Because when changing the ipsilateral modality, it triggers the contralateral callbacks, which, in turn, should trigger the ipsilateral callbacks. That's why I defined these update_without_trigger() methods.

Currently, I am thinking of an implementation similar to what I did with the transition_tensor for the Edge class that does not require us to delete these matrices. But it's a bit harder in these cases...

YoelPH commented 5 months ago

Interestingly, it does not create an infinite loop Seemingly, the change of the contralateral modalities does not trigger a second callback since only the ipsilateral should be triggering the callback. Which probably means, that when setting contralateral modalities, no callbacks are initialized and the ipsilateral side is not synced. I.e. we only have a one way syncing.

Generally I would welcome a syncing approach that is global as you implemented before.

rmnldwg commented 5 months ago

Alright, then I'll work on that now:

A two-way sync for the modalities via the callbacks and globally cached functions for the transition and observation matrices.

rmnldwg commented 5 months ago

Ok, this was a bit trickier than I thought 😅

Because the diagnose_matrices depend on three things:

  1. the modalities
  2. the data
  3. and the T-category that was requested

If either changes, it should be recomputed. So, I had to compute a hash for all three quantities (ideally somewhat fast) and combine them into one hash to make sure you always get the correct diagnose matrix.

The changes are now in the dev branch. All tests (and some new ones) pass, but I still have the feeling that there could be something wrong with this new implementation.

So, it'd be great if you could double check that you can still reproduce your results. Thanks!

YoelPH commented 5 months ago

I'll go through it on Monday to double check that everything is fine. Thanks for the changes!

YoelPH commented 5 months ago

I tested the trinary and the binary model with bilateral sampling and risk calculations. The results were good --> seems like everything works as expected. The computation time is only minimally increased (might also just be some CPU fluctuations^^). Looks like a perfect fix!