Skip to content

Fix encoded_missing_value being ignored and all-missing datetime columns crashing - #76

Open
devYRPauli wants to merge 3 commits into
google-research:mainfrom
devYRPauli:fix/encoder-missing-value-and-nat-datetime
Open

devYRPauli wants to merge 3 commits into
google-research:mainfrom
devYRPauli:fix/encoder-missing-value-and-nat-datetime

Conversation

@devYRPauli

Copy link
Copy Markdown
Contributor

Two independent edge case fixes in tabfm/src/classifier_and_regressor.py, one per commit.

1. CategoricalOrdinalEncoder ignores encoded_missing_value

encoded_missing_value is accepted in __init__ and documented as "Encoded value used for NaN / missing values", but transform() never reads it. Missing values and unseen categories are both filled with unknown_value, so the parameter has no effect.

enc = CategoricalOrdinalEncoder(unknown_value=-1, encoded_missing_value=-99)
enc.fit(np.array([["known"], ["other"]], dtype=object))
enc.transform(np.array([[np.nan], ["unseen"], ["known"]], dtype=object))
# before: [-1., -1., 0.]
# after:  [-99., -1., 0.]

The fix records the original NaN positions before mapping and fills those with encoded_missing_value, leaving unknown_value for values genuinely absent from the category map. Both parameters default to -1, so behaviour is unchanged unless a caller sets them differently.

2. DatetimeTransformer crashes on an all-missing datetime column

fit() stores series.mean() as the fill value. For a column with no valid timestamps that mean is NaT, so the later fillna is a no-op and the subsequent .astype(np.int64) on the extracted parts raises:

DatetimeTransformer().fit_transform(pd.DataFrame({"ts": pd.to_datetime([None, None, None])}))
# before: pandas.errors.IntCastingNaNError: Cannot convert non-finite values (NA or inf) to integer
# after:  completes, shape (3, 5), all values finite

The fix falls back to the epoch when the computed mean is NaT. Columns with at least one valid timestamp are unaffected.

Tests

Added CategoricalOrdinalEncoderTest and DatetimeTransformerTest to classifier_and_regressor_test.py. Both were confirmed to fail against the unpatched source (-1.0 vs -99 for the first, IntCastingNaNError for the second) and pass with the change.

Full existing suite after the change: 48 tests run, 20 passed, 28 skipped because JAX is not installed in my environment. Those 28 remain unverified locally.

These changes do not touch any line modified by my open PRs #59 or #69.

@sylvesterkaczmarek sylvesterkaczmarek left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

transform() now emits encoded_missing_value, but inverse_transform() still only excludes unknown_value from the category lookup. With the example encoded_missing_value=-99, a missing value therefore reaches cats[-99], which can raise or decode to an unrelated category. Should inverse_transform() handle the missing sentinel explicitly as well?

transform() now writes encoded_missing_value for NaN inputs, so
inverse_transform() can receive that sentinel. Its mask only excluded
unknown_value, so the sentinel reached cats[value]. With
encoded_missing_value=-99 and three categories that raises IndexError; with
120 categories it returns cats[-99], an unrelated category, and raises
nothing.

Exclude the missing sentinel from the same mask. Missing values now decode
to None, matching how unknown values already decode and what main returns
today. The defaults and both in-repo construction sites use -1 for both
sentinels, so their behavior is unchanged.
@devYRPauli

Copy link
Copy Markdown
Contributor Author

You are right. I measured both halves of it.

Before this PR transform() never emitted encoded_missing_value, so inverse_transform() only ever saw unknown_value and its mask was sufficient. This PR makes the missing sentinel reachable, and the decode path does not know about it.

Measured with CategoricalOrdinalEncoder(unknown_value=-1, encoded_missing_value=-99), one missing value in the input:

categories base main this PR
3 [None, None, 'a'] IndexError: index -99 is out of bounds for axis 0 with size 3
120 [None] ['v21'], which is cats[-99]

So both failure modes you named are real. The second one is the worse of the two, because it decodes to an unrelated category and raises nothing.

Reach inside the repo is narrower. Both construction sites pass unknown_value=-1, encoded_missing_value=-1, and the class defaults are the same pair, so the mask already covers the sentinel there. The only inverse_transform() call is on y_encoder_, and its input is np.argmax(...), which is always in [0, n_classes). The exposure is a caller who uses the public class directly with distinct sentinels.

Fixed by excluding the missing sentinel from the same mask:

valid_mask = (col != self.unknown_value) & (
    col != self.encoded_missing_value
)

Missing now decodes to None, the same as an unknown value, which matches what main returns today.

Added test_inverse_transform_does_not_decode_the_missing_sentinel. With the one-line change reverted it fails with IndexError: index -99 is out of bounds for axis 0 with size 2. The full file is 49 tests, all passing, 28 skipped for a missing JAX.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants