search

richarddmorey / BayesFactor

BayesFactor R package for Bayesian data analysis with common statistical models.
https://richarddmorey.github.io/BayesFactor/
131 stars 48 forks source link

Does `{hypergo}` need to be a `{BayesFactor}` dependency? #168

Closed IndrajeetPatil closed 8 months ago

IndrajeetPatil commented 9 months ago

Looking at the package source code, the only LOC where it is used is commented out:

https://github.com/richarddmorey/BayesFactor/blob/4165b16d05050ee24a134cbc8a2f0e95e196e4ca/pkg/BayesFactor/R/correlation-JASP.R#L23

So I am wondering if it needs to a hard dependency of {BayesFactor}?

richarddmorey commented 8 months ago

It's also here:

https://github.com/richarddmorey/BayesFactor/blob/4165b16d05050ee24a134cbc8a2f0e95e196e4ca/pkg/BayesFactor/R/ttest-informed-JASP.R#L5

It's been years since that happened, but as I recall, this is the history:

  1. I started using {hypergeo} functions
  2. I reimplemented them in c++
  3. I submitted the c++ code to the author of hypergeo, who has updated the package on GitHub (https://github.com/RobinHankin/hypergeo); I was thinking I would then start using the {hypergeo} package again, so that the code was in one maintainable place
  4. They have not yet updated the CRAN version
  5. The JASP team submitted code that uses {hypergeo} at some point, not knowing about the hypergeometric functions I've written in {BayesFactor}, which tethers it to {hypergeo}

I believe it would be straightforward to remove the dependency on {hypergeo} given that the code already exists in {BayesFactor}, but I'm not sure it would be smart given that the same code will be in the CRAN {hypergeo}, and it is always good to keep code in one place; is there a problem with having the dependency?

IndrajeetPatil commented 8 months ago

Thanks, Richard, for providing a detailed account of the evolution of this segment of the code.

is there a problem with having the dependency?

Not at all. But I created the issue only because I wasn't aware that the JASP module was also using {hypergo}, even if the R package isn't. It was just to avoid an unnecessary dependency.

I guess it would then make sense to urge the {hypergo} author to update the CRAN version so that the same functionality is not duplicated in two separate places and {BayesFactor} can remove its internal implementation in favour of {hypergo}'s.