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

Fix issue #149 #288

Closed
wants to merge 25 commits into from
Closed

Fix issue #149 #288

wants to merge 25 commits into from

Conversation

fweber144
Copy link
Collaborator

This should fix issue #149. The main problem was in fit_glm_ridge_callback() and get_contrasts_arg_list(), but some more changes were also necessary, in particular for L1 search.

IMPORTANT: This PR should be merged after PR #287. To avoid a merge conflict when doing so, I included the NEWS entry Fix GitHub issue #268. (GitHub: #287) (which also exists at PR #287) here once again.

specifically, the new `contrasts` argument is passed to argument `contrasts.arg`
of the `model.matrix()` call within `fit_glm_ridge_callback()`).
contrasts (see `options("contrasts")`) anyway, we don't need to call it
(`contrasts.arg = NULL` in the `model.matrix()` call will also create the
default contrasts).
`collapse_contrasts_solution_path()` (the aim is to get rid of
`get_contrasts_arg_list()`).
`fit_glm_ridge_callback()` because `predict.subfit()` first needs some
adjustments (so it can also deal with user-specified contrasts).
default explicitly. Also name argument `data`.
This reverts commit 68f95c3.

Reason: Deleting the response is actually necessary. Otherwise, errors like
`Error in eval(predvars, data, env) : object '.y_glm_gauss.1' not found` are
thrown.
predictors and update a related comment in the `penalty` test.
the intercept in case of a non-intercept-only model (for GLMs).

This is a fixup for commit 5be4c57.
predictors also in case of "special" formulas.
@fweber144
Copy link
Collaborator Author

I'm just realizing that you might get a merge conflict despite my attempt to avoid this. If you do, then the final NEWS.md should look like the one from this PR.

@AlejandroCatalina
Copy link
Collaborator

Merged manually.

@fweber144 fweber144 deleted the issue149_fix branch March 26, 2022 11:58
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.

None yet

2 participants