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

Allow event_tbl as input of gs_update_ahr #499

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

Conversation

LittleBeannie
Copy link
Collaborator

When updating the boundaries, there are two approaches to consider:

  • Approach 1: Input the blinded data. The current gs_update_ahr function will calculate the events for each piecewise interval. These events are then used to calculate the treatment effect (theta) and the statistical information under the alternative hypothesis (info0).
  • Approach 2: Input the events for each piecewise interval directly.

Approach 2 is particularly useful when investigating boundaries with event accrual that is faster or slower than planned, without the need for observed data. Additionally, it is beneficial when users already know the summary of event counts.

@LittleBeannie LittleBeannie self-assigned this Feb 4, 2025
@LittleBeannie LittleBeannie linked an issue Feb 4, 2025 that may be closed by this pull request
@LittleBeannie LittleBeannie marked this pull request as draft February 4, 2025 22:26
@LittleBeannie LittleBeannie marked this pull request as ready for review February 4, 2025 22:57
@LittleBeannie LittleBeannie requested a review from keaven February 4, 2025 22:58
#' gs_update_ahr(x = x, alpha = 0.05)
gs_update_ahr <- function(
x = NULL,
alpha = NULL,
ustime = NULL,
lstime = NULL,
observed_data = NULL) {
event_tbl = NULL) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

When I recommended only adding event_tbl, I didn't know that observed_data was already an option. Is this a common use case? I worry about backwards compatibility.

One option would be to start by supporting both observed_data and event_tbl, but emit a deprecation notice if a user provides observed_data. Then in a future release, we could remove observed_data completely to simplify the code.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Something like the following:

if (!is.null(observed_data)) {
  warning(
    "The argument observed_data is deprecated and will be removed in a future release.\n",
    "Please update your code to use the argument event_tbl."
  )
  event_tbl <- func_to_convert(observed_data)
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

But this is ultimately your call. I don't know how common it was to use observed_data

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thank you for your suggestions! In some situations, we may use the entire TTE dataset as observed_data. However, I've noticed that using only event_tbl without observed_data seems to be more prevalent in practice.

Considering this, I decided to focus on only takingevent_tbl. Additionally, I will create a function to convert observed_data into event_tbl, ensuring that we maintain flexibility in data handling.

Please let me know if you have any further thoughts or suggestions on this approach!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow event table as input of gs_update_ahr
2 participants