Skip to content

Conversation

@ngreifer
Copy link

Hi Henrik, This is a really cool project, and I wanted to help expand the available functions that could be futurized. I added support for my R package fwb, which is basically a drop-in for boot. This isn't really the best use case because parallelization is as simple as setting cl = "future" in the package's function calls, but I thought this would be a good excuse to better understand the package in case I make some additional contributions.

I added fwb to most of the relevant documentation (wherever boot was mentioned), including the Description and README, and I added some tests. I didn't update the NEWS or give it its own vignette; I figured you could pretty much copy the boot vignette or even add fwb to it.

Noah

@HenrikBengtsson
Copy link
Collaborator

Thank you for this! Could you please create an issue that points to this PR? I've missed, forgotten about PR-only contributions in the past.

R/addons-fwb.R Outdated

template <- quote(
local({
oopts <- options(future.ClusterFuture.clusterEvalQ = "error")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Option future.ClusterFuture.clusterEvalQ is only used when future::makeClusterFuture() is involved. It looks like you're using pbapply, which uses future.apply internally, so there's no need for this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

And because of that, there is not restriction on R (>= 4.4.0).

Copy link
Author

Choose a reason for hiding this comment

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

Got it, the change should be made!

Copy link
Collaborator

Choose a reason for hiding this comment

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

You can drop the local({ ... }) wrapper too

Copy link
Collaborator

Choose a reason for hiding this comment

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

You can drop the local({ ... }) wrapper too

@ngreifer
Copy link
Author

Mentioned in #4.

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