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

Improved power2 via furrr #15

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

danielemule
Copy link

I have modified power2() using furrr functions, as mentioned in #11.

@astamm
Copy link
Collaborator

astamm commented May 26, 2022

Automatic checks fail. Please review and correct your code.

@astamm
Copy link
Collaborator

astamm commented Jun 21, 2022

Still not working?

@astamm
Copy link
Collaborator

astamm commented Jun 24, 2022

Still not passing checks. Please check locally before pushing please.

@danielemule
Copy link
Author

danielemule commented Jun 24, 2022

I have updated DESCRIPTION file importing progressr and furrr. However, checks report:

Error: Error in loadNamespace(x) : there is no package called 'progress'

I do not explicitly use progress, should I add this package to "Imports" in DESCRIPTION file?

@astamm
Copy link
Collaborator

astamm commented Jun 24, 2022

You should check the package locally which will tell you what's wrong.
You can also explore the logs of the checks on GitHub which will also tell you what's wrong.
About package dependencies, see https://r-pkgs.org/description.html#description-dependencies.

R/simulations.R Outdated
seed = 1234
)$pvalue
)
progressr::handlers(progressr::handler_progress(format="[:bar] :percent :eta :message"))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The user defines these. Not us as package developers.

R/simulations.R Outdated
y <- furrr::future_map_dbl(1:xs, pbfun, .options = furrr::furrr_options(seed = TRUE))
}

progressr::with_progress(pvalues <- fun(R))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same thing here.
In addition, fun is not a good naming convention, nor is dummy.

R/simulations.R Outdated

fun <- function(xs) {
p <- progressr::progressor(steps = xs)
pbfun <- function(dummy){
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pbfun is not evocative as a name either...

DESCRIPTION Outdated
@@ -54,7 +54,10 @@ Imports:
magrittr,
flipr,
cli,
withr
withr,
progress,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where do you use the progress package?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As commented before:

I have updated DESCRIPTION file importing progressr and furrr. However, checks report:

Error: Error in loadNamespace(x) : there is no package called 'progress'

I do not explicitly use progress, should I add this package to "Imports" in DESCRIPTION file?

I solve errors by adding the progress package in DESCRIPTION file, but I do not use it explicitly.

@danielemule danielemule requested a review from astamm June 25, 2022 19:24
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

Successfully merging this pull request may close these issues.

2 participants