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

Unilateral risk does not provide risk array anymore #87

Closed YoelPH closed 1 week ago

YoelPH commented 1 week ago

the risk function in Unilateral does not provide an array with the risk for each state anymore. Even though lines 884-887 state, that only if we specify an involvement we marginalize, there is no if statement to do so:

        # if a specific involvement of interest is provided, marginalize the
        # resulting vector of hidden states to match that involvement of
        # interest
        return self.marginalize(involvement, posterior_state_dist)

I am not sure whether this is intended. However I prefer something like:

        if involvement != None:
            return self.marginalize(involvement, posterior_state_dist)
        return posterior_state_dist

The risk array can be quite useful for alternative analysis of the output.

rmnldwg commented 1 week ago

You can use the posterior_state_dist() method for that.

I changed it because in my opinion a risk is simply the posterior state distribution marginalized over the matching patterns of involvement. That's why the risk() method now simply calls the posterior_state_dist() and then the marginalize() method.

Previously, when calling the risk() with involvement=None, you would not get a risk, but the posterior state distribution. SO, I was hoping this is more semantic 😅

Does that work for you?

YoelPH commented 1 week ago

Sounds good! Then the output when no involvement is given should maybe give some information as well why it is just 1? Because running the risk without involvement does not make sense in this case.

rmnldwg commented 1 week ago

Hm yes, maybe... My thinking was that we typically provide None to the involvement pattern when we are not interested in a particular LNL's involvement. So, when someone provides None as the entire involvement pattern, then we just marginalize over everything 😅

But yeah, it is somewhat pointless here. We could just make the involvement argument required, not allowing it to be None.

YoelPH commented 1 week ago

Exactly. If you know about it, there is no problem. But if one sees that involvement is not required, one might be lost why there is always just 1 as output.