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

Add tests for percent #137

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

Add tests for percent #137

wants to merge 4 commits into from

Conversation

Moohan
Copy link
Member

@Moohan Moohan commented Dec 3, 2024

Some of these fail but I think they should pass.

I guess we don't necessarily want to depend on {janitor} but is there any reason not to copy the code for use here? Then we'd be confident that the functionality matched exactly.

https://github.com/sfirke/janitor/blob/main/R/round_half_up.R

Either way, I think round and signif half-up should be in their own script (and probably split the tests, too) as they're not necessarily related to percent, and we have an option to export them in the future if we want.

Some of these fail but I think they should pass.

I guess we don't necessarily want to depend on `{janitor}` but is there any reason not to copy the code for use here? Then we'd be confident that the functionality matched exactly.

https://github.com/sfirke/janitor/blob/main/R/round_half_up.R

Either way, I think round and signif half-up should be in their own script (and probably split the tests, too) as they're not necessarily related to percent, and we have an option to export them in the future if we want.
@Moohan Moohan requested a review from Nic-Chr December 3, 2024 13:32
@Nic-Chr
Copy link
Contributor

Nic-Chr commented Dec 3, 2024

Some of these fail but I think they should pass.

I guess we don't necessarily want to depend on {janitor} but is there any reason not to copy the code for use here? Then we'd be confident that the functionality matched exactly.

Because it's a single expression, R evaluates it in one pass so it's more efficient and therefore rounding percent vectors is more lightweight.

The code is mostly taken from here: https://github.com/insightsengineering/cards/blob/9939a82974e88b2f667d41ad219f9a30b247397a/R/round5.R#L24

This code is used for rounding halves up in the well-known gtsummary package so I'm quite confident it works as expected.

@Nic-Chr
Copy link
Contributor

Nic-Chr commented Dec 3, 2024

Some of these fail but I think they should pass.
I guess we don't necessarily want to depend on {janitor} but is there any reason not to copy the code for use here? Then we'd be confident that the functionality matched exactly.

Because it's a single expression, R evaluates it in one pass so it's more efficient and therefore rounding percent vectors is more lightweight.

The code is mostly taken from here: https://github.com/insightsengineering/cards/blob/9939a82974e88b2f667d41ad219f9a30b247397a/R/round5.R#L24

This code is used for rounding halves up in the well-known gtsummary package so I'm quite confident it works as expected.

Actually I think it uses 2 passes, but in any case should still be more lightweight.

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