search

lbbe-software / fitdistrplus

Help to Fit of a Parametric Distribution to Non-Censored or Censored Data
https://lbbe-software.github.io/fitdistrplus/
52 stars 10 forks source link

Extending fitdistrplus #33

Open seabbs opened 3 weeks ago

seabbs commented 3 weeks ago

Firstly, thanks for a great package.

We are currently extending fitdistrplus to support primary censored events (on top of your existing support for secondary censoring) and have got a bit stuck with the custom distribution support. The issue we are facing is assigning a custom distribution to the global environment to make it work. This works which is great but I think it won't be CRAN compliant - see https://github.com/epinowcast/primarycensoreddist/issues/62v.

Is there anything we have missed with fitdistrplus that might help us or any other suggestion to work around this?

If the answer to both of those is no then potentially this could be resolved by allowing custom family functions to be passed as arguments. If you are interested in that I would be happy to submit a PR.

mldelignette commented 2 weeks ago

Thanks for your comment, but we are not clear about the nature of the problem. Could you provide us a runnable example so that we could really understand it ? Or if it is not possible, try to give us more details in the description of the problem ? Best regards, Marie Laure

seabbs commented 2 weeks ago

Sure.

This is our current wrapper function: https://github.com/epinowcast/primarycensoreddist/blob/main/R/fitdistdoublecens.R

Note the need to use withr to dummy the global environment which has been our fix to get around this for our use case.

This is an example use case: https://github.com/epinowcast/primarycensoreddist/blob/main/R/fitdistdoublecens.R

The first example shows how to naively use fitdistrplus directly for this problem, the second shows a manual approach which I think is your recommended approach and the last shows our function wrapper approach.

The core of the problem is the design choice to rely on detecting distribution functions in the global environment as this makes extending the package difficult because you have to assign your custom distributions to the global environment. I see a few options:

  1. Do nothing to the code but perhaps tweak the docs to highlight this limitation and provide the ad-hoc solution (using withr) or a better solution if available (I don't know what this would be but perhaps we are misunderstanding).
  2. Update the internal code to target the parent frame rather than the global environment. This would remove the need for withr and make everything "just work".
  3. Update the interface to allow users to pass in custom distribution functions vs trying to detect them.

Happy to implement any of these as a PR.