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

Using furrr instead of pbapply and letting the user set the parallelization. #17

Merged
merged 9 commits into from
Jan 6, 2025

Conversation

ManonSimonot
Copy link
Collaborator

  • {pbabbly} has been deleted from the package and replaced with {furrr} and {progressr}.
  • all 'ncores' parameters have been deleted from the package: that implied changes in the compute_point_estimate() function that should be reviewed since I was not sure how this has to be done.
  • a vignette has been created to tell the user how to set parallelization with {future}, {parallel} and {progressr}: modifications may be needed.

@ManonSimonot ManonSimonot linked an issue Dec 20, 2024 that may be closed by this pull request
@astamm
Copy link
Collaborator

astamm commented Dec 30, 2024

Thanks for the PR. It is important to benchmark to ensure that parallel processing speeds computation up. This can be done in the vignette that you created to further prove to users the benefits of using parallel computations.

The compute_point_estimate() function is a work-in-progress and did not work well even before your changes. You should clean the file to remove currently commented lines and add an issue to better work out the optimization problem that this function requires to solve.

@ManonSimonot
Copy link
Collaborator Author

The vignette has been updated with a time comparison between sequential and parallel processing.
The compute_point_estimation() function is clean and an issue is open for this matter.

@astamm astamm merged commit ba23a02 into master Jan 6, 2025
7 checks passed
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.

Use future+cli package for computation instead of pbapply
2 participants