From b906b0e73e36b6592d5695f5c2e303512ec8ff39 Mon Sep 17 00:00:00 2001 From: ChrisW09 <50968720+ChrisW09@users.noreply.github.com> Date: Fri, 16 Oct 2026 14:16:28 +0200 Subject: [PATCH] fix(placement): fill tied-data locations with distinct interior points On discrete or heavily tied features the selector's quantile top-up and its quantile fallbacks collapsed onto the tied values and the range boundary. The top-up sliced the quantile candidates before de-duplicating them, did not restrict them to the open range and skipped the spacing filter, and the fallbacks returned quantile_knots() verbatim. The result was too few locations (output narrower than output_dim, and fold-dependent widths that crashed CrossFittedTransformer with a misleading "disable adaptive sizing" error), duplicate locations, and locations on x_min/x_max that gave PLE a constant column. The top-up and both fallbacks now keep every selector-found location and fill the shortfall with strictly interior, spaced quantile candidates first, then uniform ones; only a request too dense for the spacing adds the remaining uniform points without it, and a zero-range feature keeps its repeated value. LightGBM's +/-1e-35 split-at-zero sentinel is mapped to the midpoint of the values it separates, and PLE treats any positive bin width as a real bin instead of using an absolute 1e-10 tolerance that encoded narrow bins as a constant 0.5. Locations move on tied or discrete features only; continuous features with enough splits are unchanged. Fixes #57 --- docs/representations/choosing_a_method.md | 5 ++ pretab/core/selectors.py | 104 +++++++++++++++++----- pretab/encoding/numerical/ple.py | 4 +- tests/core/test_cross_fitted.py | 14 +++ tests/core/test_feature_map_selector.py | 14 +++ tests/core/test_location_selectors.py | 67 ++++++++++++++ tests/core/test_ple_selector.py | 26 ++++++ 7 files changed, 209 insertions(+), 25 deletions(-) diff --git a/docs/representations/choosing_a_method.md b/docs/representations/choosing_a_method.md index 2e457d5..4d9a6bf 100644 --- a/docs/representations/choosing_a_method.md +++ b/docs/representations/choosing_a_method.md @@ -87,6 +87,11 @@ PreTab is explicit about degenerate inputs rather than failing silently. tensor-product) raise a typed error rather than fitting a meaningless basis. Numeric binning falls back to a single bin instead of raising, and PLE and the feature maps place all their bins or centers at the same value, a degenerate but still valid basis. +- **Discrete or heavily tied column**: target-aware PLE and feature maps still get the + requested number of distinct bin edges or centers strictly inside the observed range. When + the tree finds fewer splits than requested, the shortfall is filled with spaced quantile + points and then evenly spaced points, so the output width does not depend on the data (or + on the cross-fitting fold). - **Out-of-range input at transform**: B/M/I-spline, P-spline, and tensor-product clip values beyond the fitted range by default; natural-cubic and cubic-regression extrapolate smoothly by default. Pass `policy=RepresentationPolicy(out_of_range=...)` directly to any diff --git a/pretab/core/selectors.py b/pretab/core/selectors.py index f4cef30..894ddb6 100644 --- a/pretab/core/selectors.py +++ b/pretab/core/selectors.py @@ -15,8 +15,10 @@ the optional ``lightgbm`` dependency (``pip install pretab[lightgbm]``). Both share the :class:`BaseLocationSelector` template, which handles input -validation, small-sample quantile fallbacks, minimum spacing, and topping up or -trimming the candidate set to fit within ``[min_count, max_count]``. +validation, small-sample fallbacks, minimum spacing, and topping up or trimming +the candidate set to fit within ``[min_count, max_count]``. Fallback and top-up +locations are distinct and strictly inside the feature's range, also on tied or +discrete features. """ from abc import ABC, abstractmethod @@ -26,10 +28,14 @@ from sklearn.tree import DecisionTreeClassifier, DecisionTreeRegressor from ..exceptions import IncompatibleParamsError, OptionalDependencyError -from .knots import quantile_knots, select_knots +from .knots import quantile_knots, select_knots, uniform_knots Task = Literal["regression", "classification"] +# Magnitude bound of LightGBM's "split at zero" sentinel threshold (its +# ``kZeroThreshold`` is 1e-35, reported as the float32 value ~1.0000000180e-35). +_LIGHTGBM_ZERO_THRESHOLD = 1e-34 + class BaseLocationSelector(ABC): """Abstract base class for count-based, target-aware location selectors. @@ -40,9 +46,11 @@ class BaseLocationSelector(ABC): candidate set to ``max_count`` using the selector's importance ranking). The shared :meth:`select` template validates and cleans the inputs, falls - back to quantile locations when there are too few samples or no candidates, - enforces a minimum spacing, and tops up or trims the result so it lands - inside ``[min_count, max_count]``. + back to unsupervised locations when there are too few samples or no + candidates, enforces a minimum spacing, and tops up or trims the result so it + lands inside ``[min_count, max_count]``. Fallback and top-up locations are + distinct, strictly interior points: spaced quantile locations first, then + uniform ones, so a tied or discrete feature still gets the requested count. The following attributes are expected to be set by every subclass: ``min_location_spacing`` and ``min_samples_floor``. @@ -78,7 +86,10 @@ def select( Returns ------- locations : np.ndarray - Sorted array of selected locations. + Sorted array of between ``min_count`` and ``max_count`` distinct + locations strictly inside the range of the non-missing ``x`` values. + A feature with a single distinct value has no interior; its value + is then repeated ``min_count`` times. """ if y is None: raise IncompatibleParamsError(f"{type(self).__name__} requires y to select locations.") @@ -97,11 +108,11 @@ def select( y_valid = y[valid_mask] if len(x_valid) < self.min_samples_floor: - return quantile_knots(x_valid, min_count) + return np.array(self._supplement([], x_valid, min_count)) points, context = self._ordered_candidates(x_valid, y_valid, task) if len(points) == 0: - return quantile_knots(x_valid, min_count) + return np.array(self._supplement([], x_valid, min_count)) locations = self._enforce_spacing(points, x_valid) @@ -149,22 +160,44 @@ def _enforce_spacing(self, split_points: list[float], x: np.ndarray) -> list[flo return spaced def _supplement(self, existing: list[float], x: np.ndarray, target_count: int) -> list[float]: - """Top up an under-filled location set with quantile locations. + """Top up an under-filled location set to ``target_count`` distinct locations. Keeps every existing (selector-found) location and fills only the - shortfall with quantile candidates, rather than truncating the union - (which would preferentially drop the largest existing values). + shortfall, rather than truncating the union (which would preferentially + drop the largest existing values). Candidates lie strictly inside the + range of ``x`` and keep ``min_location_spacing`` from every kept location: + evenly spread quantile locations first, then uniform ones. On a tied or + discrete feature the quantiles collapse onto a few values (often the range + boundary), and the uniform candidates still provide distinct interior + locations for any feature with a positive range. Only when the requested + count is too dense for the spacing are the remaining uniform locations + added without it. """ missing = target_count - len(existing) if missing <= 0: return existing - existing_set = set(existing) - candidates = [c for c in quantile_knots(x, target_count).tolist() if c not in existing_set] - combined = sorted(existing_set | set(candidates[:missing])) - if len(combined) > target_count: - combined = select_knots(np.array(combined), target_count).tolist() - return combined + x = np.asarray(x, dtype=float).ravel() + x_min, x_max = float(x.min()), float(x.max()) + if x_max <= x_min: + # A zero-range feature has no interior; keep the historical repeated + # location so the requested count (and output width) still holds. + return sorted([*existing, *quantile_knots(x, missing).tolist()]) + + min_distance = self.min_location_spacing * (x_max - x_min) + kept = sorted(float(location) for location in existing) + for candidates in (quantile_knots(x, target_count), uniform_knots(x, target_count)): + eligible = [] + for candidate in np.unique(candidates[(candidates > x_min) & (candidates < x_max)]): + if all(abs(candidate - other) >= min_distance for other in (*kept, *eligible)): + eligible.append(float(candidate)) + kept = sorted(kept + select_knots(np.array(eligible), missing).tolist()) + missing = target_count - len(kept) + if missing <= 0: + return kept + + leftovers = np.setdiff1d(uniform_knots(x, target_count), kept) + return sorted(kept + select_knots(leftovers, missing).tolist()) class CARTLocationSelector(BaseLocationSelector): @@ -370,16 +403,39 @@ def _trim_over_max(self, points: list[float], context: object, max_count: int) - def _extract_split_points_with_gains(self, model, x: np.ndarray) -> dict[float, float]: """Collect split thresholds and their cumulative gains from a model.""" + x = np.asarray(x, dtype=float).ravel() x_min, x_max = float(x.min()), float(x.max()) split_importance: dict[float, float] = {} + # LightGBM reports a split at zero as the sentinel threshold +/-1e-35 + # rather than as a bin midpoint. Map it to the midpoint between the + # values the split separates, the location CART would report. + zero_splits = { + 1.0: self._midpoint(x[x <= 0], x[x > 0]), + -1.0: self._midpoint(x[x < 0], x[x >= 0]), + } + model_dict = model.dump_model() for tree_info in model_dict["tree_info"]: - self._traverse_tree(tree_info["tree_structure"], split_importance, x_min, x_max) + self._traverse_tree(tree_info["tree_structure"], split_importance, x_min, x_max, zero_splits) return split_importance - def _traverse_tree(self, node: dict, split_importance: dict, x_min: float, x_max: float): + @staticmethod + def _midpoint(left: np.ndarray, right: np.ndarray) -> float | None: + """Midpoint between the largest ``left`` and the smallest ``right`` value.""" + if left.size == 0 or right.size == 0: + return None + return float((left.max() + right.min()) / 2.0) + + def _traverse_tree( + self, + node: dict, + split_importance: dict, + x_min: float, + x_max: float, + zero_splits: dict[float, float | None] | None = None, + ): """Recursively accumulate split gains for the single feature.""" if "split_feature" not in node: return @@ -387,10 +443,12 @@ def _traverse_tree(self, node: dict, split_importance: dict, x_min: float, x_max if node["split_feature"] == 0: threshold = node["threshold"] gain = node.get("split_gain", 0.0) - if x_min < threshold < x_max: + if zero_splits is not None and abs(threshold) <= _LIGHTGBM_ZERO_THRESHOLD: + threshold = zero_splits[1.0 if threshold >= 0 else -1.0] + if threshold is not None and x_min < threshold < x_max: split_importance[threshold] = split_importance.get(threshold, 0.0) + gain if "left_child" in node: - self._traverse_tree(node["left_child"], split_importance, x_min, x_max) + self._traverse_tree(node["left_child"], split_importance, x_min, x_max, zero_splits) if "right_child" in node: - self._traverse_tree(node["right_child"], split_importance, x_min, x_max) + self._traverse_tree(node["right_child"], split_importance, x_min, x_max, zero_splits) diff --git a/pretab/encoding/numerical/ple.py b/pretab/encoding/numerical/ple.py index 81a639c..cca1e86 100644 --- a/pretab/encoding/numerical/ple.py +++ b/pretab/encoding/numerical/ple.py @@ -276,7 +276,7 @@ def _apply_piecewise_linear_vectorized( if len(thresholds) == 0: lower, upper = edges[0], edges[-1] width = upper - lower - if width > 1e-10: + if width > 0: values = np.clip((feature - lower) / width, 0.0, 1.0) else: values = np.full(n_samples, 0.5) @@ -297,7 +297,7 @@ def _apply_piecewise_linear_vectorized( upper_edge = edges[bin_idx + 1] bin_width = upper_edge - lower_edge - if bin_width > 1e-10: + if bin_width > 0: ple_encoded[mask, bin_idx] = np.clip((values - lower_edge) / bin_width, 0.0, 1.0) else: ple_encoded[mask, bin_idx] = 0.5 diff --git a/tests/core/test_cross_fitted.py b/tests/core/test_cross_fitted.py index 723d971..21d9480 100644 --- a/tests/core/test_cross_fitted.py +++ b/tests/core/test_cross_fitted.py @@ -104,3 +104,17 @@ def test_invalid_task_rejected(data): X, y = data with pytest.raises(InvalidParamError, match="task"): CrossFittedTransformer(PLETransformer(), task="classificaton").fit_transform(X, y) + + +@pytest.mark.parametrize("seed", range(5)) +def test_cross_fitting_a_discrete_feature_keeps_a_fixed_width(seed): + """Regression guard for issue #57: tied features gave fold-dependent widths.""" + from pretab.transformers import RBFExpansionTransformer + + rng = np.random.default_rng(seed) + x = rng.integers(0, 6, size=200).astype(float).reshape(-1, 1) + y = np.sin(x[:, 0]) + rng.normal(0, 0.5, size=200) + out = CrossFittedTransformer( + RBFExpansionTransformer(output_dim=10, target_aware=True), n_folds=5, random_state=0 + ).fit_transform(x, y) + assert out.shape == (200, 10) diff --git a/tests/core/test_feature_map_selector.py b/tests/core/test_feature_map_selector.py index 2b886c6..20d4ac5 100644 --- a/tests/core/test_feature_map_selector.py +++ b/tests/core/test_feature_map_selector.py @@ -100,3 +100,17 @@ def test_lightgbm_selector_places_centers(Cls, data): t = Cls(output_dim=5, target_aware=True, task="regression", placement_strategy="lightgbm").fit(X, y) assert all(len(c) == 5 for c in t.centers_) assert all(np.all(np.diff(c) > 0) for c in t.centers_) + + +@pytest.mark.parametrize("strategy", ["cart", "lightgbm"]) +def test_target_aware_centers_keep_output_dim_on_a_discrete_feature(strategy): + """Regression guard for issue #57: tied features returned fewer, duplicate centers.""" + if strategy == "lightgbm": + pytest.importorskip("lightgbm") + rng = np.random.default_rng(0) + x = rng.integers(0, 5, size=300).astype(float).reshape(-1, 1) + y = (x[:, 0] >= 1) + rng.normal(0, 0.1, size=300) + transformer = RBFExpansionTransformer(output_dim=10, target_aware=True, placement_strategy=strategy).fit(x, y) + centers = transformer.centers_[0] + assert transformer.transform(x).shape[1] == 10 + assert len(np.unique(centers)) == 10 diff --git a/tests/core/test_location_selectors.py b/tests/core/test_location_selectors.py index e1e6e7f..8b30811 100644 --- a/tests/core/test_location_selectors.py +++ b/tests/core/test_location_selectors.py @@ -160,3 +160,70 @@ def test_supplement_preserves_high_end_split_end_to_end(): locations = CARTLocationSelector().select(xs.reshape(-1, 1), ys, task="regression", min_count=6, max_count=6) assert locations.max() > 5.0, f"expected a high-end location to survive supplementing, got {locations}" + + +# --- tied / discrete features (issue #57) ------------------------------------ + + +def _discrete(levels, n=300, seed=0): + rng = np.random.default_rng(seed) + x = rng.integers(0, levels, size=n).astype(float) + return x.reshape(-1, 1), (x >= 2) + rng.normal(0, 0.1, size=n) + + +def _assert_distinct_interior(locations, X, count): + assert len(locations) == count + assert len(np.unique(locations)) == count + assert locations.min() > X.min() and locations.max() < X.max() + + +@pytest.mark.parametrize("selector_cls", [CARTLocationSelector, LightGBMLocationSelector]) +@pytest.mark.parametrize("levels", [2, 3, 5]) +def test_selector_tops_up_tied_features_with_distinct_interior_locations(selector_cls, levels): + """Regression guard for issue #57: the quantile top-up collapsed onto the tied + values and the range boundary, returning too few / duplicate locations.""" + if selector_cls is LightGBMLocationSelector: + pytest.importorskip("lightgbm") + X, y = _discrete(levels) + locations = selector_cls().select(X, y, task="regression", min_count=8, max_count=8) + _assert_distinct_interior(locations, X, 8) + + +def test_supplement_on_tied_data_respects_spacing_and_count(): + x = np.repeat([0.0, 1.0, 2.0], 50) + selector = CARTLocationSelector() + result = selector._supplement([0.5], x, 6) + assert len(result) == 6 + assert 0.5 in result + assert min(np.diff(result)) >= selector.min_location_spacing * 2.0 + assert min(result) > 0.0 and max(result) < 2.0 + + +def test_supplement_fills_dense_requests_beyond_the_spacing(): + x = np.linspace(0.0, 1.0, 1000) + result = CARTLocationSelector()._supplement([], x, 150) + assert len(result) == 150 + assert len(np.unique(result)) == 150 + + +def test_small_sample_fallback_on_tied_data_is_distinct_and_interior(): + X = np.array([[0.0], [0.0], [0.0], [1.0], [1.0]]) + locations = CARTLocationSelector().select(X, np.arange(5.0), min_count=3, max_count=3) + _assert_distinct_interior(locations, X, 3) + + +def test_constant_feature_fallback_keeps_the_requested_count(): + X = np.full((40, 1), 3.0) + locations = CARTLocationSelector().select(X, np.arange(40.0), min_count=4, max_count=4) + np.testing.assert_array_equal(locations, np.full(4, 3.0)) + + +def test_lightgbm_zero_split_sentinel_maps_to_the_bin_midpoint(): + pytest.importorskip("lightgbm") + rng = np.random.default_rng(0) + x = rng.integers(0, 5, size=300).astype(float) + y = (x >= 1) + rng.normal(0, 0.1, size=300) + locations = LightGBMLocationSelector().select(x.reshape(-1, 1), y, min_count=4, max_count=4) + # The 0-vs-positive split is reported by LightGBM as threshold 1e-35. + assert 0.5 in locations + assert (np.abs(locations) > 1e-30).all() diff --git a/tests/core/test_ple_selector.py b/tests/core/test_ple_selector.py index 498b9ac..6935dfb 100644 --- a/tests/core/test_ple_selector.py +++ b/tests/core/test_ple_selector.py @@ -95,3 +95,29 @@ def test_lightgbm_selector_places_thresholds(data): t = PLETransformer(output_dim=5, task="regression", placement_strategy="lightgbm").fit(X, y) assert t.n_bins_per_feature_ == [5] assert np.all(np.diff(t.thresholds_[0]) > 0) + + +@pytest.mark.parametrize("levels", [2, 3, 4, 5, 6]) +def test_ple_keeps_output_dim_and_no_constant_columns_on_discrete_features(levels): + """Regression guard for issue #57: thresholds on the range boundary produced a + constant column and a narrower-than-requested output.""" + rng = np.random.default_rng(levels) + x = rng.integers(0, levels, size=400).astype(float).reshape(-1, 1) + y = np.sin(x[:, 0]) + rng.normal(0, 0.3, size=400) + out = PLETransformer(output_dim=7).fit(x, y).transform(x) + assert out.shape[1] == 7 + assert (out.std(axis=0) > 0).all() + + +def test_ple_encodes_the_position_inside_narrow_bins_on_a_tiny_range_feature(): + """Regression guard for issue #57: PLE treated every bin narrower than an + absolute 1e-10 as zero-width and encoded all of its values as 0.5.""" + rng = np.random.default_rng(0) + x = rng.uniform(0.0, 1e-11, size=200) + t = PLETransformer(output_dim=4).fit(x.reshape(-1, 1), rng.normal(size=200)) + edges = t.edges_[0] + bins = np.searchsorted(t.thresholds_[0], x, side="right") + expected = (x - edges[bins]) / (edges[bins + 1] - edges[bins]) + out = t.transform(x.reshape(-1, 1)) + assert out.shape[1] == 4 + np.testing.assert_allclose(out[np.arange(len(x)), bins], expected, rtol=1e-5, atol=1e-6)