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

Code errors #10

Closed
paulgovan opened this issue Mar 9, 2020 · 5 comments
Closed

Code errors #10

paulgovan opened this issue Mar 9, 2020 · 5 comments

Comments

@paulgovan
Copy link

This issue is for openjournals/joss-reviews#2111.

After taking a cursory review of the code, there seem to be problems with some of the functions. For example fit_analytic does not seem to be a recognized function. The code/documentation needs to be updated before a more in-depth review is possible.

Also, I would recommend a more description naming convention for the package. Functions like 'fit' and 'estimate' are very vague and can be confusing when working with multiple packages at the same time.

@donaldRwilliams
Copy link
Owner

donaldRwilliams commented Mar 9, 2020

fit_analytic is not a function in the package, so not really sure how to address this. If you are referring to the example in estimate.R, then fit_analytic is the name of an object--i.e.,

#' # analytic approach (sample by setting analytic = FALSE)
#' fit_analytic <- estimate(Y, analytic = TRUE)

Note also we do not have a function named fit. The function estimate estimates GGMs (and note changing names of functions would be quite the change considering the package is already on CRAN, researchers use it, etc.)

@paulgovan
Copy link
Author

The examples in the README should be updated so that they are stand alone. For example, the fit_analytic object is not defined in the README, so these examples are not reproducible. I would also recommend checking the other documentation for similar issues.

@donaldRwilliams
Copy link
Owner

I see that now. I will fix it and check all the examples !

@jayrobwilliams
Copy link

Another option would be to include fit_analytic as a data object in the package, as bfi is currently. I'll also note that this issue is included in the paper as well as the README.

@donaldRwilliams
Copy link
Owner

Hi, yes, I fixed this.

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

No branches or pull requests

3 participants