-
Notifications
You must be signed in to change notification settings - Fork 7
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
WIP: Rename smooth_terms to additional_terms #53
base: main
Are you sure you want to change the base?
Conversation
Could you please add some tests? |
55f791b
to
b52e57f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that this PR is currently removing functionality introduced in #45, and I am not sure why.
@@ -99,48 +94,17 @@ build_formula <- function(target, covariates, smooth_terms = NULL, group_by = "g | |||
#' k = "auto") | |||
#' } | |||
fit_gam <- function(df_tract, | |||
target = NULL, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that we do want to allow target to be NULL for the case that the user provides a formula.
This is the test that is currently failing on the CI.
method="fREML", | ||
...) { | ||
|
||
# Check that if formula is non-NULL, all the other formula-setting inputs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this code being removed? This code is here to deal with the two options: provide a formula, or let the code build the formula from the inputs. Unless I am missing something, I don't think that the current PR should be eliminating these options.
Resolves #52