diff --git a/CHANGELOG.md b/CHANGELOG.md index fbc30c5..988541f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -113,6 +113,34 @@ adheres to [Semantic Versioning](https://semver.org/). entirely of complex values is still declined before parsing and keeps its dtype. +- The report no longer calls a datetime parse "converted to object". A column + of datetime strings carrying different UTC offsets — `"2021-01-05 + 00:00:00+01:00"`, `"2021-01-06 00:00:00+02:00"`, the ordinary result of a + daylight-saving changeover or of merging exports from two regions — is parsed + cell by cell into timestamps that each keep their own offset. No single + `datetime64` dtype can hold several offsets, so pandas returns them in an + `object` column. That parse is faithful and lossless, but `fix_dtypes` built + its action description from the resulting dtype alone and recorded it as + `converted to object`, risk low, which reads as though nothing was + converted. A column mixing values with and without an offset took the same + path and got the same record. The description now says what happened: + `parsed to timestamps; kept as object because the values carry different + UTC offsets, so no single datetime64 dtype can hold them` (or `... because + the values mix timezone-aware and timezone-naive timestamps, ...`), followed + by the existing ` (N unparseable value(s) set to missing)` suffix when cells + were coerced. The coercion warning for such a column likewise says the + values "could not be parsed as timestamps" instead of "as object". Affects + every supported pandas version (pandas 1.x delivers `datetime.datetime` + cells, pandas 2 `Timestamp` cells; the record is the same). + + **Compatibility impact:** report text only. Cell values, dtypes, risk + levels and confidence are unchanged, and so is every other column's + `converted to ` description, including single-offset + (`datetime64[ns, UTC+01:00]`) and naive (`datetime64[ns]`) datetime + columns. Code that matched the literal string `converted to object` for + such a column needs to match the new description. `fd.profile` still + previews such a column as `would convert to object`. + ### Documentation diff --git a/src/freshdata/steps/dtypes.py b/src/freshdata/steps/dtypes.py index a581263..9d57032 100644 --- a/src/freshdata/steps/dtypes.py +++ b/src/freshdata/steps/dtypes.py @@ -592,8 +592,56 @@ def _warn_type_contamination(col: str, s: pd.Series, config: CleanConfig, COERCED_CELLS_CAP = 1_000 +def _is_object_datetime(target: str, converted: pd.Series) -> bool: + """A datetime parse that pandas could only deliver as an object column. + + Values carrying different UTC offsets, or mixing timezone-aware with + timezone-naive values, have no common ``datetime64`` dtype, so pandas + returns one timestamp object per cell in an ``object`` column. The cells + were parsed; only the dtype is not a datetime64 one. + """ + return target == "datetime" and not is_datetime64_any_dtype(converted.dtype) + + +def _object_datetime_reason(converted: pd.Series) -> str: + """Say why the parsed timestamps in *converted* share no datetime64 dtype.""" + aware = naive = False + offsets = set() + for value in converted.dropna(): + utcoffset = getattr(value, "utcoffset", None) + if utcoffset is None: + continue + offset = utcoffset() + if offset is None: + naive = True + else: + aware = True + offsets.add(offset) + if aware and naive: + why = "the values mix timezone-aware and timezone-naive timestamps" + elif len(offsets) > 1: + why = "the values carry different UTC offsets" + else: + return "no single datetime64 dtype can hold them" + return f"{why}, so no single datetime64 dtype can hold them" + + +def _describe_conversion(target: str, converted: pd.Series) -> str: + """The audit description of what :func:`fix_dtypes` did to a column. + + ``"converted to "`` except when a datetime parse ended in an object + column: saying "converted to object" there reads as though nothing was + converted, when every cell was in fact parsed to a timestamp. + """ + if _is_object_datetime(target, converted): + return (f"parsed to timestamps; kept as {converted.dtype} because " + f"{_object_datetime_reason(converted)}") + return f"converted to {converted.dtype}" + + def _record_coerced(col: str, before: pd.Series, converted: pd.Series, - report: CleanReport, config: CleanConfig) -> None: + report: CleanReport, config: CleanConfig, + target: str = "") -> None: """Preserve the original value of every cell the conversion nulled. For declared ``sensitive_columns`` the row keys survive but every value is @@ -615,9 +663,11 @@ def _record_coerced(col: str, before: pd.Series, converted: pd.Series, for i, v in list(originals.head(3).items())) truncated = "" if len(originals) <= COERCED_CELLS_CAP else ( f"; first {COERCED_CELLS_CAP} recorded") + parsed_as = ("timestamps" if _is_object_datetime(target, converted) + else str(converted.dtype)) report.add_warning( f"column '{col}': {len(originals)} value(s) could not be parsed as " - f"{converted.dtype} and were set to missing — e.g. {examples}. " + f"{parsed_as} and were set to missing — e.g. {examples}. " f"Originals are preserved in report.coerced_cells{truncated}; these " "cells stay missing (never auto-imputed) so they can be reviewed." ) @@ -726,10 +776,10 @@ def fix_dtypes(df: pd.DataFrame, config: CleanConfig, report: CleanReport) -> pd if converted is None: _warn_type_contamination(str(col), s, config, report) continue - description = f"converted to {converted.dtype}" + description = _describe_conversion(target, converted) if n_coerced: description += f" ({n_coerced} unparseable value(s) set to missing)" - _record_coerced(str(col), s, converted, report, config) + _record_coerced(str(col), s, converted, report, config, target) report.add("fix_dtypes", description, column=str(col), count=int(converted.notna().sum()) + n_coerced) df[col] = converted diff --git a/tests/test_dtypes_threshold_mutants.py b/tests/test_dtypes_threshold_mutants.py index 95ed909..4722f31 100644 --- a/tests/test_dtypes_threshold_mutants.py +++ b/tests/test_dtypes_threshold_mutants.py @@ -482,20 +482,21 @@ def test_the_post_semantic_retry_tolerates_exactly_the_casualty_budget(): # --------------------------------------------------------------------------- -def test_mixed_utc_offsets_are_reported_as_a_conversion_to_object(): - """Pins current behaviour found while covering bool#16 -- a defect. +def test_mixed_utc_offsets_parse_to_an_object_column_of_timestamps(): + """Pins current behaviour found while covering bool#16. Values carrying *different* UTC offsets cannot share a datetime64 column, - so pandas returns an object-dtype Series of Timestamps. ``_try_datetime`` - measures that object Series against the threshold, passes it, and the - column is announced as a dtype conversion: the report says "converted to - object" and the frame keeps an object column. A conversion that ends in - ``object`` is not a conversion, and because the result is not - datetime64 the ambiguity note in ``_record_coerced`` can never fire for - such a column either. - - Nothing here argues the behaviour is right; it is pinned so that fixing - it has to change this test deliberately. + so pandas returns an object-dtype Series of per-cell timestamps, each + keeping its own offset. ``_try_datetime`` measures that object Series + against the threshold, passes it, and the column keeps the parsed + timestamps in an object column. + + The audit record used to say "converted to object", which reads as though + nothing was converted (FD2-015); it now says what happened. The exact + wording is pinned in ``tests/test_mixed_offset_audit_record.py``. Whether + such a column should be converted at all (``utc=True``, or declined) is + not decided here; the dtype is pinned so that changing it has to change + this test deliberately. """ values = [ "2021-01-05 00:00:00+01:00", @@ -510,4 +511,5 @@ def test_mixed_utc_offsets_are_reported_as_a_conversion_to_object(): report = CleanReport() frame = fix_dtypes(pd.DataFrame({"when": values}), CleanConfig(), report) assert str(frame["when"].dtype) == "object" - assert [a.description for a in report.actions] == ["converted to object"] + [description] = [a.description for a in report.actions] + assert description.startswith("parsed to timestamps; kept as object") diff --git a/tests/test_mixed_offset_audit_record.py b/tests/test_mixed_offset_audit_record.py new file mode 100644 index 0000000..6cba7c0 --- /dev/null +++ b/tests/test_mixed_offset_audit_record.py @@ -0,0 +1,281 @@ +"""The audit record of a datetime parse that pandas can only keep as ``object``. + +Datetime strings carrying *different* UTC offsets (or mixing offset-bearing +and offset-free values) have no common ``datetime64`` dtype. pandas parses +every cell to its own timestamp, each keeping its own offset, and returns them +in an ``object`` column. That parse is faithful and lossless, and ``fix_dtypes`` +delivers it -- but it used to record the action as ``"converted to object"``, +which reads as though nothing was converted (FD2-015). + +These tests pin the record, not the data: the description now says the +values were parsed to timestamps and why they stay ``object``, while the cell +values, the dtype, the risk and the confidence stay exactly what they were. +Every other column keeps its ``"converted to "`` description. +""" +from __future__ import annotations + +import datetime as dt + +import pandas as pd +import pytest + +import freshdata as fd +from freshdata.config import CleanConfig +from freshdata.report import CleanReport +from freshdata.steps.dtypes import ( + fix_dtypes, + refine_numeric_after_semantic, + suggest_conversion, +) + +MIXED_OFFSETS = [ + "2021-01-05 00:00:00+01:00", + "2021-01-06 00:00:00+02:00", + "2021-01-07 00:00:00+03:00", + "2021-01-08 00:00:00+04:00", +] + +#: Enough distinct values that one junk cell stays under the 95% threshold. +MIXED_OFFSETS_WITH_JUNK = [ + f"2021-01-{d:02d} 00:00:00+0{d % 5}:00" for d in range(1, 26) +] + ["garbage"] + +NAIVE_AND_AWARE = [ + "2021-01-05 00:00:00", + "2021-01-06 00:00:00+01:00", + "2021-01-07 00:00:00", + "2021-01-08 00:00:00", +] + +DIFFERENT_OFFSETS = ( + "parsed to timestamps; kept as object because the values carry different " + "UTC offsets, so no single datetime64 dtype can hold them" +) +NAIVE_MIXED_WITH_AWARE = ( + "parsed to timestamps; kept as object because the values mix " + "timezone-aware and timezone-naive timestamps, so no single datetime64 " + "dtype can hold them" +) +ONE_CASUALTY = " (1 unparseable value(s) set to missing)" + + +def _fix(values): + """``fix_dtypes`` on one column named ``t``: ``(frame, report)``.""" + report = CleanReport() + frame = fix_dtypes(pd.DataFrame({"t": values}), CleanConfig(), report) + return frame, report + + +def _only_action(report: CleanReport): + [action] = [a for a in report.actions if a.step == "fix_dtypes"] + return action + + +# --------------------------------------------------------------------------- +# The record says what happened +# --------------------------------------------------------------------------- + + +def test_mixed_offsets_are_recorded_as_parsed_timestamps_kept_as_object(): + frame, report = _fix(MIXED_OFFSETS) + assert str(frame["t"].dtype) == "object" + action = _only_action(report) + assert action.description == DIFFERENT_OFFSETS + assert action.column == "t" + assert action.count == 4 + + +@pytest.mark.parametrize( + "values", + [ + pytest.param( + ["2021-01-05T10:00:00+01:00", "2021-03-05T10:00:00+01:00", + "2021-07-05T10:00:00+02:00", "2021-08-05T10:00:00+02:00"], + id="dst-changeover", + ), + pytest.param( + ["2021-01-05T10:00:00Z", "2021-01-06T10:00:00Z", + "2021-01-07T10:00:00+01:00", "2021-01-08T10:00:00Z"], + id="z-and-offset", + ), + pytest.param( + ["2021-01-05 00:00:00.123456+01:00", "2021-01-06 00:00:00.5+02:00", + "2021-01-07 00:00:00+03:00", "2021-01-08 00:00:00+04:00"], + id="fractional-seconds", + ), + ], +) +def test_every_different_offset_shape_gets_the_same_record(values): + frame, report = _fix(values) + assert str(frame["t"].dtype) == "object" + assert _only_action(report).description == DIFFERENT_OFFSETS + + +def test_mixed_offsets_in_a_string_dtype_column_get_the_same_record(): + frame, report = _fix(pd.Series(MIXED_OFFSETS, dtype="string")) + assert str(frame["t"].dtype) == "object" + assert _only_action(report).description == DIFFERENT_OFFSETS + + +def test_naive_mixed_with_aware_values_names_that_reason(): + """Same mechanism, different cause: some values have no offset at all.""" + frame, report = _fix(NAIVE_AND_AWARE) + assert str(frame["t"].dtype) == "object" + assert _only_action(report).description == NAIVE_MIXED_WITH_AWARE + + +def test_the_public_clean_report_carries_the_record(): + out, report = fd.clean(pd.DataFrame({"t": MIXED_OFFSETS}), + return_report=True, verbose=False) + assert str(out["t"].dtype) == "object" + assert [a.description for a in report.actions if a.step == "fix_dtypes"] == [ + DIFFERENT_OFFSETS + ] + + +def test_risk_and_confidence_are_not_changed_by_the_new_record(): + action = _only_action(_fix(MIXED_OFFSETS)[1]) + assert action.description == DIFFERENT_OFFSETS + assert (action.risk, action.confidence) == ("low", 1.0) + + +def test_description_helper_falls_back_to_a_generic_reason(): + """No probed input reaches this branch; it keeps the record truthful anyway.""" + from freshdata.steps.dtypes import _describe_conversion # noqa: PLC0415 + + dates = pd.Series([dt.date(2021, 1, 5), dt.date(2021, 1, 6)], dtype=object) + assert _describe_conversion("datetime", dates) == ( + "parsed to timestamps; kept as object because no single datetime64 " + "dtype can hold them" + ) + # A non-datetime target keeps the plain dtype description. + assert _describe_conversion("numeric", dates) == "converted to object" + + +# --------------------------------------------------------------------------- +# (c) casualties keep their suffix and their warning +# --------------------------------------------------------------------------- + + +def test_unparseable_suffix_is_kept_after_the_new_description(): + frame, report = _fix(MIXED_OFFSETS_WITH_JUNK) + assert str(frame["t"].dtype) == "object" + assert _only_action(report).description == DIFFERENT_OFFSETS + ONE_CASUALTY + assert report.coerced_cells == {"t": {25: "garbage"}} + [warning] = report.warnings + assert warning.startswith( + "column 't': 1 value(s) could not be parsed as timestamps and were set " + "to missing — e.g. 'garbage' (row 25)." + ) + + +def test_naive_and_aware_casualty_keeps_its_suffix(): + values = [ + f"2021-02-{d:02d} 00:00:00" + ("+01:00" if d % 2 else "") for d in range(1, 26) + ] + ["garbage"] + _, report = _fix(values) + assert _only_action(report).description == NAIVE_MIXED_WITH_AWARE + ONE_CASUALTY + + +# --------------------------------------------------------------------------- +# (a) controls: every datetime64 conversion keeps its description exactly +# --------------------------------------------------------------------------- + + +def test_single_offset_column_keeps_its_description(): + values = [v[:-6] + "+01:00" for v in MIXED_OFFSETS] + frame, report = _fix(values) + dtype = frame["t"].dtype + assert isinstance(dtype, pd.DatetimeTZDtype) + # ``UTC+01:00`` on pandas 2, ``pytz.FixedOffset(60)`` on pandas 1.x. + assert _only_action(report).description == f"converted to {dtype}" + + +def test_naive_column_keeps_its_description(): + frame, report = _fix([v[:10] for v in MIXED_OFFSETS]) + assert str(frame["t"].dtype) == "datetime64[ns]" + assert _only_action(report).description == "converted to datetime64[ns]" + + +def test_naive_column_with_a_casualty_keeps_its_description_and_warning(): + values = [f"2021-01-{d:02d}" for d in range(1, 26)] + ["garbage"] + frame, report = _fix(values) + assert str(frame["t"].dtype) == "datetime64[ns]" + assert _only_action(report).description == ( + "converted to datetime64[ns]" + ONE_CASUALTY + ) + [warning] = report.warnings + assert "could not be parsed as datetime64[ns] and were set to missing" in warning + + +def test_single_offset_column_with_a_casualty_keeps_its_description(): + values = [f"2021-01-{d:02d} 00:00:00+01:00" for d in range(1, 26)] + ["garbage"] + frame, report = _fix(values) + dtype = frame["t"].dtype + assert isinstance(dtype, pd.DatetimeTZDtype) + assert _only_action(report).description == f"converted to {dtype}" + ONE_CASUALTY + + +@pytest.mark.parametrize( + ("values", "expected"), + [ + (["1", "2", "3", "4.5"], "converted to float64"), + (["yes", "no", "yes", "no"], "converted to bool"), + ], +) +def test_non_datetime_conversions_keep_their_description(values, expected): + assert _only_action(_fix(values)[1]).description == expected + + +# --------------------------------------------------------------------------- +# (b) the data is exactly what it was before the record changed +# --------------------------------------------------------------------------- + + +def test_mixed_offset_cells_are_unchanged(): + """Pinned against main's output: one timestamp per cell, own offset kept.""" + frame, _ = _fix(MIXED_OFFSETS) + column = frame["t"] + assert str(column.dtype) == "object" + assert [cell.isoformat() for cell in column] == [ + "2021-01-05T00:00:00+01:00", + "2021-01-06T00:00:00+02:00", + "2021-01-07T00:00:00+03:00", + "2021-01-08T00:00:00+04:00", + ] + assert all(isinstance(cell, dt.datetime) for cell in column) + # Exactly the series ``suggest_conversion`` produced: the record is the + # only thing fix_dtypes adds. + target, converted, _ = suggest_conversion(pd.Series(MIXED_OFFSETS), CleanConfig()) + assert target == "datetime" + assert column.equals(converted) + assert [type(c) for c in column] == [type(c) for c in converted] + + +def test_mixed_offset_cells_with_a_casualty_are_unchanged(): + frame, _ = _fix(MIXED_OFFSETS_WITH_JUNK) + column = frame["t"] + assert str(column.dtype) == "object" + assert [cell.isoformat() for cell in column.iloc[:-1]] == [ + f"2021-01-{d:02d}T00:00:00+0{d % 5}:00" for d in range(1, 26) + ] + assert column.iloc[-1] is pd.NaT + + +# --------------------------------------------------------------------------- +# The post-semantic numeric retry cannot produce this situation +# --------------------------------------------------------------------------- + + +def test_semantic_numeric_retry_leaves_parsed_timestamps_alone(): + """``refine_numeric_after_semantic`` only ever yields int64/Int64/float64. + + Even handed the object column of timestamps directly, nothing parses as a + number, so it declines and records nothing -- its own "converted to ... + after semantic repair" description never sees an object result. + """ + frame, _ = _fix(MIXED_OFFSETS) + report = CleanReport() + out = refine_numeric_after_semantic(frame.copy(), ["t"], CleanConfig(), report) + assert report.actions == [] + assert out["t"].equals(frame["t"])