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

diagnose time prior updating is not checked #53

Closed YoelPH closed 7 months ago

YoelPH commented 8 months ago

There is an issue when sampling since NaN values are produced in the likelihood. I pinned down the problem to the diagnose time file.

in line 188 we have:

         self._kwargs.update({p: kwargs[p] for p in params_to_set})

which means that we do not check whether the parameter is in an allowed range. I exchanged it with:

           for p in params_to_set:
                if not 0. <= kwargs[p] <= 1.:
                    raise ValueError("diagnose probability must be between 0 and 1!")
                self._kwargs.update({p: kwargs[p]})
rmnldwg commented 8 months ago

True, this is a problem for sampling.

However, the proposed solution would limit all parameters of any distribution to be between 0 and 1. Instead, I now delegated the responsibility to define parameter bounds to the user who defines the parametric function: It should raise a ValueError (as it should anyways) when invalid parameters are provided. This exception is the propagated in the set_params() method and eventually leads the likelihood() method to return -np.inf.

Do you think that solution will work?