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

Refactor S3 methods for fixed designs #482

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

Conversation

jdblischak
Copy link
Collaborator

This is a proof-of-concept PR related to our discussion of the classes in #457

Instead of using conditional statements via if/else/switch to mimic OOP, I converted summary.fixed_design(), as_gt.fixed_design(), and as_rtf.fixed_design() to instead use hierarchical S3 classes to dispatch the correct function for the given "method", eg "ahr".

I'll explain how this works using as_gt(). When calling as_gt() on the result of a summary() of a design object, it will first call its method-specific as_gt(). In the case of data produced by fixed_design_ahr(), this will be as_gt.design_fixed_ahr_summary(). This function sets the method-specific title and footnote, and then passes this information via NextMethod() to the more general as_gt.design_fixed_summary(), which contains the actual {gt} code to produce the table. summary() and as_rtf() work in similar ways, defining the method-specific data and then passing this to the generic function used by all methods.

Here is an example with ahr:

devtools::load_all(".")

# ahr
x <- fixed_design_ahr(
 alpha = .025,
 enroll_rate = define_enroll_rate(duration = 18, rate = 20),
 fail_rate = define_fail_rate(
   duration = c(4, 100),
   fail_rate = log(2) / 12,
   hr = c(1, .6),
   dropout_rate = .001
 ),
 study_duration = 36
)
class(x)
## [1] "design_fixed_ahr" "fixed_design"     "list"
xsum <- summary(x)
class(xsum)
## [1] "design_fixed_ahr_summary" "design_fixed_summary"     "fixed_design"
## [4] "tbl_df"                   "tbl"                      "data.frame"
as_gt(xsum)
as_rtf(xsum, file = "refactor.rtf")

Pros:

  • Using S3 methods is more transparent than changing the behavior of a function internally by inspecting the class of the object
  • Shouldn't affect end user code since this only affects the names of the classes
  • The default arguments for each method are now displayed to the end user in the man page instead of hidden in the source code

image

Cons:

  • More code duplication. For example, if we wanted to add a new argument to as_gt(), we'd have to add it multiple places

Other notes:

  • If we like this strategy, the idea would be to apply similar changes to to_integer() and the gs_design S3 generics. I started with the fixed_design S3 generics since they were the simplest.
  • I updated as_rtf() so that the superscript "{a}" is always added, even for custom titles and footnotes. In the current version, the superscript is lost when providing a custom title or footnote. I think my update is an improvement, and thus propose we update the snapshot tests
  • The actual class names are just suggestions. I'm fine with whatever naming strategy as long as it is consistent
  • Note that I also created different classes for the output of summary(). It doesn't make sense for both the input and output object of summary() to have the same classes when they are fundamentally different (ie we have designed as_gt() to only work on the output from summary(), and it shouldn't be applied to the output from fixed_design_ahr())

@jdblischak jdblischak self-assigned this Oct 30, 2024
@keaven keaven marked this pull request as ready for review October 30, 2024 23:24
@jdblischak
Copy link
Collaborator Author

Counter proposal from @yihui:

The method-specific S3 functions are mainly defining strings like title and footnote. An alternative would be to have these strings be defined upstream by the function that generates the trial data, eg gs_design_ahr(), and then saved as object attributes. Then functions like summary() and as_gt() would read these attributes to set title and footnote, unless they are explicitly provided by the user as function arguments.

Main advantage over my current proposal in this PR: Much less boiler plate code. We'd have a single summary.fixed_design() and as_gt.fixed_design() which would obtain the method-specific details from the object attributes (so it still eliminates the current practice of querying the class of the object)

Main disadvantage of my current proposal in this PR: The default values are less transparent. When defined as the default arguments to a method-specific S3 function, then the default values are documented in the man page. When specified as attributes, a user would have to find the default values via the source code or by querying the hidden attributes.

My thoughts:

  • I think the decision between these alternatives hinges on how often an average user needs to modify the default values. If this is rare, then it's not a big deal for the default values to be less accessible. But if is common, I think defining the defaults in the arguments to method-specific functions is worth the extra code
  • We need to keep in mind that I started with the fixed_design methods because they were the simplest. Thus I agree that just setting things like title and footnote appears trivial, and could be easily achieved via other means. The gs_design class and to_integer() are more complex, so it may make more sense to use S3 for these

@yihui
Copy link
Contributor

yihui commented Oct 31, 2024

The current approach brings too much boilerplate code, which makes me feel like shooting a mosquito with a canon (S3), but you may be right that S3 could start to show real value as we work on other functions. We'll see.

I'm not sure if users will ask the question "where did the default title/footnote come from?". The scenario may be more like: users generate a table or summary, see the automagic default title/footnote, and either accept them with no questions asked, or provide their own titles/footnotes via arguments. The origin of the defaults is a implementation detail that's not worth their attention. Transparency can be helpful, but I don't know how valuable it is to show these lengthy strings on the help pages.

I think the path will clearer as you move deeper into the forest. I can't make a clear judgment at the moment.

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