search

luismurao / bamm

Dynamic Distribution Modeling using the BAM scheme
1 stars 0 forks source link

Undo future::plan() the right way #1

Closed HenrikBengtsson closed 1 year ago

HenrikBengtsson commented 1 year ago

Hi and congrats to your CRAN submission.

As the author of Futureverse, I've got one comment. Please make sure to undo any changes to future::plan() and, when you do, make sure it's reset to the same as the user has set before, which may not be sequential. See Section 'For package developers' in https://future.futureverse.org/reference/plan.html for how to do this properly. This will avoid surprising R users and their use of Futureverse elsewhere.

I've spotted a few places where this needs to be updated:

https://github.com/luismurao/bamm/blob/0b4698045d5e2bdd0197a06c682ec6bdd00e37c2/R/models2pam.R#L49-L61

https://github.com/luismurao/bamm/blob/0b4698045d5e2bdd0197a06c682ec6bdd00e37c2/R/models2pam.R#L78-L92

https://github.com/luismurao/bamm/blob/0b4698045d5e2bdd0197a06c682ec6bdd00e37c2/R/null_dispersion_field_distribution.R#L43-L59

Thanks

luismurao commented 1 year ago

Thank you @HenrikBengtsson! I wasn't aware of this. Please could you check if I did it right?

HenrikBengtsson commented 1 year ago

Yes, commit 2b5eed96 looks correct to me. The other thing I looked at was to make sure that you don't have additional on.exit() calls without add = TRUE following these. So, all LGTM.

luismurao commented 1 year ago

@HenrikBengtsson checked, thanks again! I am closing the issue