-
Notifications
You must be signed in to change notification settings - Fork 10
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
Add linearpredictor
and linearpredictor!
#19
Conversation
Codecov Report
@@ Coverage Diff @@
## main #19 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 2 2
Lines 36 36
=========================================
Hits 36 36
Continue to review full report at Codecov.
|
8971228
to
1f88a50
Compare
src/regressionmodel.jl
Outdated
linearpredictor!([storage,] model::RegressionModel) | ||
|
||
In-place version of [`linearpredictor`](@ref), storing the result in `storage` or | ||
updating `model` directly, if applicable. |
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.
What does updating model directly do? Do you have any examples of this?
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 was thinking of a case in which the relevant storage lives inside the model object itself. BetaRegression does this, as does GLM, though GLM exposes it in a roundabout way by passing the field as the first argument. I could also imagine a situation where updating a model directly involves mutating multiple internal fields, in which case you couldn't pass only the mutated field as the first argument since there'd be more than one.
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.
But when the linear predictor is already stored in the model, linearpredictor
can just return it directly, there's no need to call that operation linearpredictor!
, right?
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 guess what I was thinking of is if you store the model matrix, coefficients, and linear predictor in the model object, you'd call linearpredictor!
to basically mul!(linearpredictor(model), modelmatrix(model), coef(model))
or whatever is appropriate for the model. GLM calls this linpred!
but as I said exposes the update a bit differently even though everything actually is stored in the model object. So perhaps it's my description in the docstring that's inaccurate (or at least imprecise) in terms of what I meant with this function.
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.
OK. I still don't really see what linearpredictor!(model)
is supposed to do. :-)
I imagine the idea is to follow the pattern used here?
https://github.com/ararslan/BetaRegression.jl/blob/beb83b5ad0c9acbf8cee19138395a49d66be774e/src/BetaRegression.jl#L150
If so, my suggestion is to simply use linearpredictor(model)
for that, as the fact that internal fields are mutated is an implementation detail (as long as users are clearly told that the returned vector is owned by the model).
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.
Yeah, that's the pattern I was referring to, which I was imagining could similarly be adopted in GLM to simplify some calls. For BetaRegression, as long as the linear predictor is stored in the model I want to keep linearpredictor
purely as an accessor as it's used that way extensively (currently as linpred
) inside the package. In the meantime I've removed the bit in the documentation here about a single argument.
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.
OK, let's go with with this simple version for now then.
I've added a default definition for
linearpredictor
that requires that a model has implementedmodelmatrix
,coef
, andoffset
. I did not add a default definition nor documented signature forlinearpredictor!
.As separate commits, I've also bumped the version to 1.5.0 for a feature release (assuming this is desirable) and ignored Manifest.toml in the .gitignore.