Closed YoelPH closed 5 months ago
I found the mistake. The code below, in analogy to these lines, is wrong because this
my_list = (
[1,2,3] if truthy_variable else []
+ [4,5,6] if truthy_variable else []
)
is actually equivalent to
my_list = (
[1,2,3] if truthy_variable else [] + [4,5,6] if truthy_variable else []
)
which means that the + [4,5,6] ...
is part of the else
statement and NOT a new line that gets executed regardless of the first if-else
block.
I corrected it to
my_list = (
([1,2,3] if truthy_variable else [])
+ ([4,5,6] if truthy_variable else [])
)
To make sure the stuff in the parentheses gets evaluated first.
This also explains why everything worked fine until you set the tumor spread to be symmetric: If the tumor spread is not symmetric, everything after the first else
got executed, which was coincidentally the intended outcome.
The fix is still in the dev
branch for now.
Perfect, thank you very much! I assumed the problem to be deeper inside the code, as somehow the edges still got assigned new spread probabilities. Edit: now I can not recreate the correct assignment of spread probabilities on the contralateral side without updating anymore. I probably got confused there... :)
What cannot be recreated now? Is there still an issue with the assignment/synchronization of the parameters?
Okay, I just tried your fix, unfortunately the problem persists. The bug you fixed did not solve the underlying problem. If I reassign ipsilateral tumor spread parameters, the parameters in the contralateral side still change, but the transition tensor is not updated... (I meant that I could not recreate my problem from before, but now that I looked at it more thoroughly I found the same issue as before).
My test is the following: First I create a graph with all needed parameters
graph = {
('tumor', 'primary') : ['II', 'III', 'IV'],
('lnl' , 'II') : ['III'],
('lnl' , 'III'): ['IV'],
('lnl' , 'IV') : []
}
symmetry = {'modalities': True, 'tumor_spread': True}
model = lymph.models.Bilateral(graph_dict= graph,unilateral_kwargs={'allowed_states':[0,1,2], 'max_time':15}, is_symmetric= symmetry)
model.modalities['clinical_consensus'] = lymph.modalities.Clinical(specificity = 0.94,sensitivity = 1, is_trinary = True)
def binom_pmf(k: np.ndarray, n: int, p: float):
"""Binomial PMF"""
if p > 1. or p < 0.:
raise ValueError("Binomial prob must be btw. 0 and 1")
q = (1. - p)
binom_coeff = factorial(n) / (factorial(k) * factorial(n - k))
return binom_coeff * p**k * q**(n - k)
def late_binomial(support: np.ndarray, p: float = 0.5) -> np.ndarray:
"""Parametrized binomial distribution."""
return binom_pmf(support, n=support[-1], p=p)
max_t = 15
model.diag_time_dists["early"] = sp.stats.binom.pmf(np.arange(max_t+1), max_t, 0.3)
model.diag_time_dists["late"] = late_binomial
model.get_params(as_dict=True)
Then I assign parameters
starting_points = {'ipsi_primary_to_II_spread': 0.5,
'ipsi_primary_to_III_spread': 0.07,
'ipsi_primary_to_IV_spread': 0.2,
'II_to_III_spread': 0.3,
'III_to_IV_spread': 0.2,
'late_p': 0.9}
now we can check a transition tensor:
model.contra.graph.edges['primary_to_III'].transition_tensor
The transition tensor is now correctly produced with the current parameters. If I change the transition probability from primary_to_III
, the edge
parameter is adjusted, but the tensor is still the one from the old parameter.
I found the "new" problem.
Assigning as ipsi_primary_to_III_spread': 0.07
only updates the Tensor on the contralateral side at first assignment. If I use: primary_to_III_spread': 0.07
, i.e. not using ipsi
in the key, the contralateral tensor is updated. I do not understand why a tensor deletion callback is not triggered, even though the parameter for the contralateral edge is updated, but with "correct" assignment the problem can be circumvented.
Oh good Lord 😅
Alright, thanks for the thorough investigation. I think I have an idea of how to solve it... 🤔
I think I found a good solution that also simplifies the code somewhat:
Instead of keeping a transition_tensor
stored in each Edge
instance, I simply defined a function (outside of the Edge
class) that computes this tensor based on the few parameters stored in the Edge
instance. This function is globally cached, meaning if two Edge
instances have the same parameters, they get the same tensor (no matter if they are synched or not).
This way, there is no need anymore to remember deleting and recreating the tensor. This global function always gets called. But it is not always executed, because of the global caching.
I have merged these changes into the dev
branch. Could you verify that I didn't break anything else?
If it worked, I might consider the same approach for other things that are now locally stored, deleted, and recreated.
This sounds like an amazing idea! Tomorrow I will merge your changes with my code and test the essentials and make it compatible with my midline code. I think after this step we can discuss whether the midline model I implemented fits nicely into the existing code base. (even though the caching might need to be overworked as well, as I modelled it according to the bilateral code)
First test on the binary midline model were successful. I am finishing the trinary analysis, but it looks like it works as intended 👍🏽 Edit risk and likelihood calculations seem to be identical. Further, sampling provides very similar results, which indicates correct assignment after repeated assignments.
I found another issue which I can't quite figure out yet.
In the
bilateral.py
there is the option to set up tumor edge symmetry (e.g. for a "tumor on the midline" model). However, assigning new base spread parameters (after setting them initially) does not trigger a deletion of the_transition_tensor
in the edges connecting tumor to LNL. Interestingly, this issue does not show up for edges which connect LNLs (when setting LNL transition symmetry). I tried to figure out whether there is a difference how the edges are treated/whether there is a different callback between tumor edges and LNL edges, but found none. The parameters are correctly assigned to the contralateral edges, but the callback seems not to function properly. I will try to find the origin of this bug and report it here.