[python-package] Improve pandas categorical test coverage - #7376
Conversation
maxzw
changed the title
jameslamb
left a comment
jameslamb
left a comment
There was a problem hiding this comment.
Thanks for splitting out these tests into a separate PR. Very supportive of doing things this way and I'll try to get you quick reviews so you can keep making progress on the larger project(s) you're working on.
Left a few suggestions.
| ref_valid_ds = lgb.Dataset(ref_valid_df, label=[0, 1, 0], reference=train_ds, params=dummy_dataset_params()) | ||
| ref_valid_ds.construct() | ||
|
|
||
| assert_datasets_equal(tmp_path, valid_ds, ref_valid_ds) |
There was a problem hiding this comment.
This tests that these Datasets are equal but they could also be equal in the case of a bug where these inputs were all handled incorrectly. For example, if they were all converted to floats and treated as continuous variables.
I think this test should be strengthened with some assertions that the variables were handled in the expected way.
Like:
- both columns were detected as categorical
- the encodings in
Datasetare as expected (I think you could see this in a model file if you added anlgb.train()to this or output ofDataset._dump_text(), don't recall exactly and can't spend the time to look right now)
There was a problem hiding this comment.
I've rewritten the test a little bit to improve the strictness.
- Aligned the (relative) values and categories between ordered / unordered to emphasize the differences in how they are handled.
- Check more metadata (e.g.
pandas_categoricalandparams["categorical_column"]) - Inspect the resulting encodings from
_data_from_pandasdirectly (so python-side, but I've also kept the C++ side verification with a comment explaining why the output is expected)
There was a problem hiding this comment.
Yep that's good for this PR, much stricter, thank you.
jameslamb
left a comment
jameslamb
left a comment
There was a problem hiding this comment.
Just one more small recommendation, then I think this is ready to merge.
| pandas_categorical=train_ds.pandas_categorical, | ||
| )[0] | ||
| assert valid_df_encoded[:, 0].tolist() == [0.0, 1.0, 3.0] # a -> 0, b -> 1, d -> 3 | ||
| assert valid_df_encoded[:, 1].tolist() == [0.0, 1.0, 3.0] # e -> 0, f -> 1, h -> 3 |
There was a problem hiding this comment.
Could you please update this to some example that doesn't happen to encode to identical values?
That'd help us catch issues like the categorical code leakage you mentioned over in #7343 (comment)
jameslamb
left a comment
jameslamb
left a comment
There was a problem hiding this comment.
Looks good to me, we can merge this once you pull in latest main and fix merge conflicts.
Thanks! Done 👍🏻 |
2 participants
This PR introduces two new tests related to how pandas handles categoricals, which are necessary to detect regression when implementing backend-agnostic categorical handling (#7343):
test_pandas_categorical_with_missing_valuesThis test checks that pandas
null|NaNvalues in categorical columns are not interpreted as distinct categories but are handled like missing values.test_pandas_categorical_encoding_registered_but_unobservedThis test verifies the encoding behavior when categorical values are defined in the dtype but never appear in training data. For unordered categoricals, such values map to NaN (the C++ bin mapper has no bin for them). For ordered categoricals (treated as numeric), such values interpolate or clip to the nearest observed bin boundary. This matches standard continuous feature behavior for out-of-range values.