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

assertion for tablePattern #24

Merged
merged 4 commits into from
Mar 27, 2024
Merged

assertion for tablePattern #24

merged 4 commits into from
Mar 27, 2024

Conversation

grewered
Copy link
Collaborator

added assertion for vector x.

I wasn't sure about the rest of the function.
does pattern need assertions? And what kind of values should be allowed/sensible?

what does useNA do? with useNA = c("no", "ifany", "always")) as default.
It's not used in the example, so do you even change that argument?
do you use of of the three options, and shall i write a test, whether one of them has been chosen - or just leave it be?

Copy link
Owner

@weirichs weirichs left a comment

Choose a reason for hiding this comment

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

checkmate::assert_vector(x) is fine.

pattern may need assertions if it is not null. So you can add after if(length(pattern)>0) { a line with checkmate::assert_vector(pattern)

useNA is an argument which is directly passed to the table function. This argument is not changed within the function, so it does not need assertation.

In general, a possible test that the user chooses one of the three options (where abbreviations are allowed), is:

useNA <- match.arg(useNA, choices = c("no", "ifany", "always"))

But again, that's not necessary here.

If weights is not missing (i.e. line 24), you might add an assertation that weights is a numeric vector with length equal to the length of x.

@grewered grewered requested a review from weirichs March 18, 2024 11:34
@weirichs weirichs self-assigned this Mar 25, 2024
Copy link
Owner

@weirichs weirichs left a comment

Choose a reason for hiding this comment

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

LGTM, in general.
I don't know whether it's possible, but pattern should be a vector with unique elements, i.e. something like pattern = c(1, 2, 3, 1) is inappropriate. I don't know whether checkmate provides corresponding assertations. If not, everything can stay as it is.
Alternatively, you could add pattern <- unique(pattern) between line 7 and 8.

I'm not sure whether you should point out to users that pattern must be unique, or whether you should change this silently in the function code.

What would you prefer?

@grewered grewered requested a review from weirichs March 25, 2024 14:39
@grewered
Copy link
Collaborator Author

like this? checking for uniqueness with checkmate is usually very easy.

Copy link
Owner

@weirichs weirichs left a comment

Choose a reason for hiding this comment

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

Very well, thanks!

@weirichs weirichs merged commit 1aeca82 into master Mar 27, 2024
8 checks passed
@weirichs weirichs deleted the check_tablePattern branch March 27, 2024 10:59
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