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 info_scale documentation #203

Merged
merged 23 commits into from
Apr 17, 2023
Merged

Conversation

LittleBeannie
Copy link
Collaborator

No description provided.

@LittleBeannie LittleBeannie linked an issue Mar 24, 2023 that may be closed by this pull request
@LittleBeannie LittleBeannie self-assigned this Mar 24, 2023
@LittleBeannie LittleBeannie added the documentation Improvements or additions to documentation label Mar 24, 2023
@LittleBeannie LittleBeannie requested a review from nanxstats March 24, 2023 15:43
Copy link
Collaborator

@nanxstats nanxstats left a comment

Choose a reason for hiding this comment

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

It is an anti-pattern to define info_scale = c(0, 1, 2) and have the logic below:

info_scale <- if (methods::missingArg(info_scale)) {
  2
} else {
  match.arg(as.character(info_scale), choices = 0:2)
}

because you are trying to change the actual default to 2 from the percepted default 0 in convention.

If you want to use 2 as the default, then the best way is to define info_scale = c("2", "0", "1") and do this in the function body

info_scale <- match.arg(info_scale)

Please take a look at https://design.tidyverse.org/def-enum.html and update the argument definition and logic.

@LittleBeannie
Copy link
Collaborator Author

It is an anti-pattern to define info_scale = c(0, 1, 2) and have the logic below:

info_scale <- if (methods::missingArg(info_scale)) {
  2
} else {
  match.arg(as.character(info_scale), choices = 0:2)
}

because you are trying to change the actual default to 2 from the percepted default 0 in convention.

If you want to use 2 as the default, then the best way is to define info_scale = c("2", "0", "1") and do this in the function body

info_scale <- match.arg(info_scale)

Please take a look at https://design.tidyverse.org/def-enum.html and update the argument definition and logic.

Thanks for the suggestion! Just correct in the following commit.

Copy link
Collaborator

@nanxstats nanxstats left a comment

Choose a reason for hiding this comment

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

OK. I think we are getting close.

First, the canonical use of match.arg() is to match values in a character vector, not a numeric vector. Second, the list is not long, so there is no need to use choices.

To me, this would be ideal:

function (info_scale = c("2", "0", "1")) {
    info_scale <- match.arg(info_scale)
}

The options are indeed character strings instead of numeric values --- although they look like numeric.

The long-term solution is to have better names for these options, instead of using "0", "1", "2", if that makes sense.

@LittleBeannie
Copy link
Collaborator Author

although they look like nu

Thanks for the suggestion! If we have function (info_scale = c("2", "0", "1")) {info_scale <- match.arg(info_scale)}, do user input info_scale = 1 or info_scale = "1"? The latter might not be straightforward since it quotes a numerical number...

@nanxstats
Copy link
Collaborator

Thanks for the suggestion! If we have function (info_scale = c("2", "0", "1")) {info_scale <- match.arg(info_scale)}, do user input info_scale = 1 or info_scale = "1"? The latter might not be straightforward since it quotes a numerical number...

Good thinking, but just because something is shorter or easier to type doesn't always mean it's better: it needs to be correct first. Feeling unnatural to quote them is merely a symptom. The root cause is that these options should be named in words instead of numbers. To address this now, I can propose to rename the options as:

c("combined_variance", "null_variance", "alternative_variance")

Any thoughts?

Copy link
Collaborator

@nanxstats nanxstats left a comment

Choose a reason for hiding this comment

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

Now everything builds, let's merge this.

The check errors are all fixed. They were simply due to some info_scale == conditions still using 0, 1, 2 on RHS. The error messages were misleading.

A quick note on the enumerated option names we agreed:

"h0_h1_info", "h0_info", "h1_info" correspond to the original options 2, 0, 1.

function (info_scale = c("h0_h1_info", "h0_info", "h1_info")) {
  info_scale <- match.arg(info_scale)
  ...
}

@nanxstats nanxstats merged commit 9e7356d into main Apr 17, 2023
@nanxstats nanxstats deleted the 200-gs_power_ahr-info_scale-argument branch April 17, 2023 01:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

gs_power_ahr() info_scale argument
2 participants