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

Consider updating to an object_descriptor syntax for variable names (Similar to function names) #55

Open
EeethB opened this issue Aug 17, 2023 · 3 comments

Comments

@EeethB
Copy link
Collaborator

EeethB commented Aug 17, 2023

Expanded discussion from the first comment of test_calculate_power.docx, sent on 8/16/2023 by @xidongdxi

Currently variable names follow a descriptor_object pattern - adjusted_p, sim_corr, weighting_strategy, etc. The main exception is many of the power variables, which are power_type instead (at_least_1_power just sounds really awkward to me). Variables may fit better with the function name pattern to switch these around and go object_descriptor. The object_verb(_descriptor) pattern is nice for consistency, but it's especially helpful for auto-complete (e.g. type graph and every graph-related function pops up).

Another thing to consider, specifically for power, is the pattern to group argument names by test vs simulation parameters. Focusing on grouping would result in the following. This pattern is most helpful for auto-complete (i.e. type test and get all the test-related parameters). I find this helpful with so many arguments, but it may be less relevant for regular users.

graph,
test_alpha,                   # Could be just alpha, to match other functions
test_groups,
test_types, 
test_corr,
sim_n,
sim_power_marginal, # Switch to power_marginal to match other power variables
sim_corr,
sim_success,
sim_seed,
force_closure,             # This argument may go away before release
verbose

Switching to the object_descriptor pattern yields this.

graph,
alpha,
groups_test,
types_test,                  # Maybe just test_types still - this style feels really awkward
corr_test,
n_sim,
power_marginal,
corr_sim,
success,
seed,
force_closure,
verbose
@xidongdxi
Copy link
Collaborator

Many thanks Ethan. Here is my preferred option.
graph,
alpha,
power_marginal,
test_groups,
test_types,
test_corr,
sim_n,
sim_corr,
sim_success,
sim_seed, # I am wondering if we need this since I realize that gMCP::calcPower does not have it. I think it is a good practice to set seed for the entire code, not necessarily for this specific one.
force_closure, # This argument may go away before release
verbose

@EeethB
Copy link
Collaborator Author

EeethB commented Aug 23, 2023

Thank you, I've made this change

@xidongdxi While we're switching up argument names, what would you think of changing the test specification arguments in graph_test_closure() to match graph_calculate_power()? i.e. corr --> test_corr and groups --> test_groups

@xidongdxi
Copy link
Collaborator

looks good to me!

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

No branches or pull requests

2 participants