Parameter assignment in bilateral model

[YoelPH]

There is a small issue in bilateral.py: https://github.com/rmnldwg/lymph/blob/dev/lymph/models/bilateral.py

in Line 343 we have:

        remaining_args, remainings_kwargs = self.contra.assign_params(
            *remaining_args, **contra_kwargs, **remainings_kwargs
        )

This does not work properly with the trinary model, where we need to assign "global" parameters like growth. A fix would be to replace **remaining_kwargs with general_kwargs. As follows:

        remaining_args, remainings_kwargs = self.contra.assign_params(
            *remaining_args, **contra_kwargs, **general_kwargs
        )

by doing so, the general kwargs like universal growth, microscopic spread and I also guess the time distribution prior is passed to both ipsi and contralateral model.

[YoelPH]

I found a second bug in the bilateral code in the comp_posterior_joint_state_dist function. In line 605 we multiply by the ipsilateral diagnose vector. Which is correct, but the transposition of the matrix does not have the desired effect. To reach the desired effect we need to build a different vector array.

Current state:

        joint_state_dist = self.comp_joint_state_dist(t_stage=t_stage, mode=mode)
        # matrix with P(Zi=zi,Zc=zc|Xi,Xc) * P(Xi,Xc) for all states Xi,Xc.
        joint_diagnose_and_state = (
            diagnose_given_state["ipsi"].T
            * joint_state_dist
            * diagnose_given_state["contra"]
        )

Potential fix:

joint_state_dist = self.comp_joint_state_dist(t_stage='early', mode='HMM')
# matrix with P(Zi=zi,Zc=zc|Xi,Xc) * P(Xi,Xc) for all states Xi,Xc.
joint_diagnose_and_state = (
    diagnose_given_state["ipsi"][:, np.newaxis] 
    * joint_state_dist
    * diagnose_given_state["contra"]
)

[rmnldwg]

I moved the comment about the bug in the multiplication of matrices into a new issue (#61).

Regarding the assignment of the parameters in the bilateral model: Right now one would need to use it like so

global_growth = 0.2
bilateral_model.assign_params(
    ipsi_growth=global_growth,
    contra_growth=global_growth,
)

to assign both sides the same growth probabilities. And if bilateral_model.symmetries["lnl_spread"] is True, then it's actually enough to just set the ipsilateral ones, as they will be synced. So, I guess there's technically no need for this, as you could define that in your custom likelihood function.

But do you think there is also the need for, e.g., keyword arguments that get copied and dispatched to both sides? For example any keyword argument starting with "bilateral_" will be sent to both assign_params() methods?

[rmnldwg]

But thinking about it again, I do think it somewhat useless how these general_kwargs are used 😅

[YoelPH]

But do you think there is also the need for, e.g., keyword arguments that get copied and dispatched to both sides? For example any keyword argument starting with "bilateral_" will be sent to both assign_params() methods?

Imo this would be the most intuitive solution. But of course what you wrote is a good option as well. I was just thinking about using the general_kwargs since they have limited usecases in the moment :)

[rmnldwg]

Or actually, I think you're right: Anything that is NOT prefixed with "ipsi_" or "contra_" should just go to both sides. So, I'll implement your proposed fix.