Slighltly improve coefnames with no intercept#301
Conversation
Adopt improvements from `termnames`.
| """ | ||
| StatsAPI.coefnames(t::FormulaTerm) = (coefnames(t.lhs), coefnames(t.rhs)) | ||
| StatsAPI.coefnames(::InterceptTerm{H}) where {H} = H ? "(Intercept)" : [] | ||
| StatsAPI.coefnames(::InterceptTerm{H}) where {H} = H ? "(Intercept)" : String[] |
There was a problem hiding this comment.
Tangential as this code is predates this PR but it's kind of odd IMO that coefnames can return a String rather than a container. The name is plural so it sounds like it should give you some collection of names, even if said collection has one element (or none).
There was a problem hiding this comment.
I would not be opposed to always returning a container, even when it's just one element, but that would be technically breaking for coefnames albeit not termnames (because `termnames hasn't appeared in a release yet)
There was a problem hiding this comment.
It's true that the name is plural, but I'm not sure returning a single string is a problem in practice. Are people really likely to call this on objects which could either be single- or multiple-term? It's not super practical either to get a single-element vector when you know in advance there will be a single value, and in terms of our own implementation it's quite wasteful (though it probably doesn't matter much).
There was a problem hiding this comment.
teh return type is already all over the place since the FormulaTerm one returns a tuple, so I'm not too fussed about it.
There was a problem hiding this comment.
I guess that means the current docstring is wrong though...
There was a problem hiding this comment.
I agree with @ararslan — always returning a container would be the best choice (and that’s what Julia does everywhere). And yes, it matters for developers that use coefnames inside their functions (as I needed to work around that in my packages)
kleinschmidt
left a comment
kleinschmidt
left a comment
There was a problem hiding this comment.
I'm fine with this overall, just a minor tweak to be super pedantic about the possible return values.
| Return value is either a `String`, an iterable of `String`s or the empty vector | ||
| if there is no associated column (e.g. `coefnames(InterceptTerm{false}())`). |
There was a problem hiding this comment.
| Return value is either a `String`, an iterable of `String`s or the empty vector | |
| if there is no associated column (e.g. `coefnames(InterceptTerm{false}())`). | |
| Return value is either a `String`, an iterable of `String`s, the empty vector | |
| if there is no associated column (e.g. `coefnames(InterceptTerm{false}())`), or | |
| (in the case of a `FormulaTerm`) a two tuple of any of the above (corresponding | |
| to `coefnames` applied to the left- and right-hand sides of the formula). |
There was a problem hiding this comment.
What if I copied and adapted the docstring for termnames(t::FormulaTerm) instead? My goal was to make the two functions consistent, and the behavior for formulas seems different enough to deserve a separate docstring.
|
@nalimilan I promised to review this one. I changed the same line in #334, so is it just the docstring update that remains here after the rebase? |
|
Yeah that's really minor now. |
6 participants
Adopt improvements from
termnames(#299).