Create feature names for non-pandas input, too #655
Conversation
in the non-pandas case
I still think it should be a waring, but first some downstream changes are in order in tabmat so that it can handle this case gracefully.
MartinStancsicsQC
requested review from
tbenthompson,
MarcAntoineSchmidtQC,
xhochy,
jtilly and
lbittarello
as code owners
src/glum/_glm.py
Outdated
| return X, y, sample_weight, offset, weights_sum, P1, P2 | ||
|
|
||
| def _get_feature_names(self, X: ArrayLike): |
There was a problem hiding this comment.
Could this method get a different name, please? I associated "get" with a low overhead getter (i.e. just return an attribute). This method doesn't return anything and it actually sets two fitted attributes.
I would think it's a bit cleaner if this method just returned the feature names and terms and leave setting them to _set_up_and_check_fit_args.
There was a problem hiding this comment.
Thanks, totally agreed. Does this seem better?
|
The proposed tabmat formula interface has a |
|
Re. parametrizing categorical column names: I agree that this is a nice feature, but this should ideally be a separate PR. |
| """ | ||
| Get feature names for the input data. | ||
|
|
||
| Stores them in the ``feature_names_`` and ``column_names_`` attributes. |
There was a problem hiding this comment.
| Stores them in the ``feature_names_`` and ``column_names_`` attributes. | |
| Stores them in the ``feature_names_`` and ``term_names_`` attributes. |
There was a problem hiding this comment.
Actually, this function doesn't store them. We should revise the docstring.
| feature_names.append(column) | ||
| term_names.append(column) | ||
|
|
||
| elif isinstance(X, tm.StandardizedMatrix): |
There was a problem hiding this comment.
We should try to minimize the amount of custom logic depending on the tabmat type. Could you instead create a new method in tabmat to deal with this? I think it would actually be very useful metadata to store in the tabmat object itself.
|
It will probably be superseded by Tabmat PR 278. Closing it for now. |
Checklist
CHANGELOG.rstentryMain changes
feature_names_attribute is populated for non-pandas input, too. It uses theX_{i}format for numerical columns, and (in the case of aSplitMatrix) theC_{i}__{cat}format for categorical columns. It would be a breaking change for users who relied on thefeature_names_attribute being unassigned in the case of non-pandas input, but I cannot imagine how that would be usefu (you never know, though).term_names_attribute is also assigned during fit. It contains the name of the column in the original input data that corresponds to each column of the design matrix. In the case of numerical variables, it is the same asfeature_names_. In the case of categorical columns, it differs in that it does not include the category at the end.Reason
A separate PR for Wald-tests will soon follow, for which it will be useful to have a simple way to refer to columns by names. The same goes for what I call terms here. I am submitting this as a separate PR as it is relatively self-contained and it might be easier to review.