Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extending fitdistrplus #33

Open
seabbs opened this issue Sep 30, 2024 · 2 comments
Open

Extending fitdistrplus #33

seabbs opened this issue Sep 30, 2024 · 2 comments

Comments

@seabbs
Copy link

seabbs commented Sep 30, 2024

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
Copy link
Collaborator

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
Copy link
Author

seabbs commented Oct 8, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants