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

Vignette upgrades #2

Merged
merged 6 commits into from
Sep 6, 2024
Merged

Vignette upgrades #2

merged 6 commits into from
Sep 6, 2024

Conversation

pegeler
Copy link
Owner

@pegeler pegeler commented Aug 18, 2024

No description provided.

@pegeler pegeler requested a review from kampfsca August 28, 2024 22:15
@pegeler pegeler marked this pull request as ready for review August 29, 2024 03:19
Copy link
Collaborator

Choose a reason for hiding this comment

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

The utilization of uniroot is cool. I wonder if some of the functions in the vignette could be integrated into the package proper.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I'd be interested in doing that. The way it's set up now is a little cumbersome because we would need pass a lot of information to the partially applied function via closure. I pull from the global environment here because it's in a controlled environment. Another approach that might be cleaner would be to take in an already computed power.cmh object and get everything needed from there. I'll think on that but I'd really like to prioritize getting these vignettes polished up so that I can submit them as a paper before adding any new functionality.

PR is welcome if you want to do it. Otherwise one of us can make an issue?

Copy link
Collaborator

@kampfsca kampfsca left a comment

Choose a reason for hiding this comment

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

Mostly cosmetic changes - we're adding spaces after commas now? Switching from sapply to vapply for a few functions is a nice upgrade. Looks solid.

@kampfsca kampfsca merged commit 7778db0 into master Sep 6, 2024
@pegeler
Copy link
Owner Author

pegeler commented Sep 7, 2024

Mostly cosmetic changes - we're adding spaces after commas now? Switching from sapply to vapply for a few functions is a nice upgrade. Looks solid.

Yes, mostly cosmetic changes, plus the little uniroot trick. Most of this is ground work for combining all three vignettes into a nicely typeset paper. Adding spaces after commas is the "correct" style these days; I would have caught that if I was running this through a linter.

Feel free to make a PR adding yourself as a "ctb" or "rev"

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