-
-
Notifications
You must be signed in to change notification settings - Fork 190
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 #1457 (projpred newdata
requiring more variables than necessary)
#1459
Conversation
This is achieved by not ignoring `extract_y` anymore (ignoring this was already undesirable from a conceptual point of view) and by setting `check_response` and `req_vars` appropriately.
… fix later in projpred's unit tests.
Thanks! Why should weights be required if the response is not? They are only relevant for log_lik, which requires the response as well. Posterior_predict for example does not make use of weights. |
I was also thinking about whether the requirement of weights could be tied to the requirement of the response, but projpred uses the weights in Yesterday, this made me think about observation weights in I think incorporating observation weights in But honestly, I'm not very keen to implement this in projpred soon, as this is a very special case and probably hard to implement in a sensible way in terms of the UI (for example, how to allow users to identify which columns of the |
I have to add that for |
I guess that |
Okay, so what we need for this PR is that variables that are listed in |
Well actually, I am not sure I want to change this stuff right now and I will have to think about whether this makes any sense at all. I will merge the PR and we can revisit the weight stuff later. |
Thanks! Revisiting the weights stuff later is ok for me. |
…_vars = character()` Thus, we need to set `req_vars` to the response variable.
…eq_vars = character()` Thus, we need to set `req_vars` to the response variable.
This fixes issue #1457. See the commit messages for details. If you don't want to increase the version number, you can revert that of course.
I have also added a
TODO
comment in this line because I experienced a slight inconsistency while testing this fix: If weights (i.e., fromresp_weights()
) are missing innewdata
(and listed inreq_vars
), no error is triggered, but such an error is triggered if numbers of trials (i.e., fromresp_trials()
) are missing innewdata
(and listed inreq_vars
). Already from a conceptual point of view, I think it would be desirable to also throw an error if weights are missing. Tell me if you need a reprex for this (but I think this would be a different issue and furthermore, this is not projpred-related).