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

Use function factory pattern to pass cutting functions to sim_gs_n() #201

Merged
merged 13 commits into from
Feb 23, 2024

Conversation

jdblischak
Copy link
Collaborator

Follow-up to #195. Closes #196.

This PR makes the following updates:

  • Instead of passing a list of quoted get_analysis_date() expressions that then need to be evaluated, the argument cutting of sim_gs_n() now accepts a list of cutting functions. These functions are created by the function factory create_cutting()
  • Instead of passing quoted test functions to the argument test of sim_gs_n(), only the test function itself is passed, and any further arguments are passed to the testing function via ...
  • Various other code improvements

Note that I didn't fully convert maxcombo(). I don't think I fully understand the purpose of this function. Its arguments are quoted functions, but they are never evaluated. Instead their arguments for rho and gamma are extracted and passed to fh_weight(). Is there any reason that maxcombo() couldn't instead just receive the vectors of rho and gamma directly? And why is maxcombo() needed when fh_weight() already exists?

@jdblischak jdblischak self-assigned this Feb 22, 2024
@LittleBeannie
Copy link
Collaborator

LittleBeannie commented Feb 22, 2024

Thanks for applying the function factory, @jdblischak ! Before the merge, could you please introduce us to the changes and new syntax tomorrow?

@nanxstats
Copy link
Collaborator

Overall looking good. Let's try follow the testing best practices in designing your test suite:

  • Avoid the library(gsDesign2) call and qualify namespaces explicitly.
  • Move the logic at top of the test file with "test file scope" into a function within a "helper" file and call the function to setup things at the start of individual tests.

@jdblischak
Copy link
Collaborator Author

Before the merge, could you please introduce us to the changes and new syntax tomorrow?

@LittleBeannie Yes, of course. I'll explain everything in today's meeting. Also note that this morning I also converted the maxcombo() interface to accept vectors instead of quoted function expressions.

@jdblischak
Copy link
Collaborator Author

Overall looking good. Let's try follow the testing best practices in designing your test suite:

@nanxstats I switched to gsDesign2:: and created helper functions defined in a separate helper file. Please let me know if you'd like me to make any further modifications.

Copy link
Collaborator

@LittleBeannie LittleBeannie left a comment

Choose a reason for hiding this comment

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

Thanks for these terrific improvement, @jdblischak ! I will add an example of maxcombo test after this PR is merged.

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.

Awesome! I love simple, reliable, and effective solutions like this.

@nanxstats nanxstats merged commit caadb3e into Merck:main Feb 23, 2024
7 checks passed
@jdblischak jdblischak deleted the function-factory branch February 23, 2024 19:33
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.

Refactor sim_gs_n() to improve interface and performance
3 participants