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

Bug in family$mu_fun() (or in a call)? #243

Closed
fweber144 opened this issue Nov 10, 2021 · 3 comments
Closed

Bug in family$mu_fun() (or in a call)? #243

fweber144 opened this issue Nov 10, 2021 · 3 comments

Comments

@fweber144
Copy link
Collaborator

Sorry, I currently don't have the time to create a reprex for this. But an inspection of the code might already be sufficient for confirming whether this is a bug or not.

Consider lines

projpred/R/cv_varsel.R

Lines 413 to 415 in ef3cfc2

mu_k <- family$mu_fun(submodels[[k]]$sub_fit,
obs = inds,
offset = refmodel$offset)
and the definition of family$mu_fun() in

projpred/R/refmodel.R

Lines 631 to 637 in ef3cfc2

family$mu_fun <- function(fit, obs = NULL, newdata = NULL, offset = NULL) {
newdata <- fetch_data(data, obs = obs, newdata = newdata)
if (is.null(offset)) {
offset <- rep(0, nrow(newdata))
}
family$linkinv(proj_predfun(fit, newdata = newdata) + offset)
}
Then shouldn't the offsets also be reduced to the observations from obs? This could be done either inside of family$mu_fun() or in the call to it (for the latter, it would make sense to add an input check to family$mu_fun() which tests in case of !is.null(offset) whether offset is of length 1 or nrow(newdata) (after newdata has been pre-processed)).

Compare with lines

mu <- family$mu_fun(sub_fit,
obs = test_points,
offset = refmodel$offset[test_points])
which indicate that from the two proposed ways for reducing the offsets, it's probably the latter one (i.e., in the call to family$mu_fun()) which is intended by projpred.

@fweber144 fweber144 changed the title Bug in family$mu_fun() (or in its calls)? Bug in family$mu_fun() (or in a call)? Nov 10, 2021
@fweber144
Copy link
Collaborator Author

I think a basic input check for length(offset) would also be desirable for situations like in lines

projpred/R/methods.R

Lines 182 to 183 in ef3cfc2

mu <- proj$family$mu_fun(proj$sub_fit,
newdata = newdata, offset = offsetnew)
, i.e., where newdata is specified.

@fweber144
Copy link
Collaborator Author

This is indeed a bug, as can be seen from debugging the call to cv_varsel() for a tstsetup which has validate_search = FALSE in the test "setting nloo smaller than the number of observations works" (and omitting the suppressWarnings() call around cv_varsel()).

fweber144 added a commit to fweber144/projpred that referenced this issue Jan 6, 2022
AlejandroCatalina pushed a commit that referenced this issue Jan 8, 2022
@fweber144
Copy link
Collaborator Author

Fixed in develop, so a corresponding label may be added here.

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

No branches or pull requests

1 participant