From c620d2b794136ff5703045828e064d00e63da245 Mon Sep 17 00:00:00 2001 From: ChrisW09 <50968720+ChrisW09@users.noreply.github.com> Date: Mon, 27 Jul 2026 22:23:33 +0200 Subject: [PATCH] fix: correct three small defects from the audit 1. ``resolve_locations`` sorted and de-duplicated ``locs`` but passed the caller's original-order ``importance`` straight through to ``trim_to_count``, so index i of one no longer referred to entry i of the other and the trim kept the wrong locations: locations [5, 1, 3] with importance [0.1, 9.9, 0.2], max_count=1 -> [3.] (expected [1.], the high-importance entry) Carry ``importance`` through the same reordering, and raise on a length mismatch. Currently unreachable -- nothing in the package calls ``resolve_locations``, it is only re-exported -- but it is public API. 2. ``BasePreTabTransformer.get_feature_names_out`` used ``zip(..., strict=False)``, so a wrong-length ``input_features`` silently produced a truncated name array instead of raising. Validate the length and raise ``InvalidParamError``, matching scikit-learn's convention. 3. ``LanguageEmbeddingTransformer.fit`` called ``X.shape[1]``, which raises ``AttributeError`` on a plain list -- including the list in its own docstring example, hidden behind ``# doctest: +SKIP``. Normalize with ``np.asarray`` first, as ``transform`` already does. The fourth item in the issue (doubled ``num_feature_0__feature_0`` prefixes for ndarray input) is left alone: it is cosmetic, and changing the synthesized stems would change public output names. Closes #21 Co-Authored-By: Claude Opus 5 (1M context) --- pretab/core/base.py | 16 ++- pretab/core/locations.py | 22 +++- .../embeddings/language_transformer.py | 5 +- tests/test_minor_defects.py | 112 ++++++++++++++++++ 4 files changed, 150 insertions(+), 5 deletions(-) create mode 100644 tests/test_minor_defects.py diff --git a/pretab/core/base.py b/pretab/core/base.py index dbeab6b..b02109a 100644 --- a/pretab/core/base.py +++ b/pretab/core/base.py @@ -11,6 +11,7 @@ from sklearn.utils.validation import check_is_fitted from .adaptive import AdaptiveResolutionMixin +from .exceptions import InvalidParamError from .params import AliasResolverMixin from .validation import validate_2d_allow_nan @@ -46,10 +47,23 @@ def _output_sizes(self) -> list[int]: raise NotImplementedError def get_feature_names_out(self, input_features=None): - """Return output feature names of the form ``{feature}_{suffix}{j}``.""" + """Return output feature names of the form ``{feature}_{suffix}{j}``. + + Raises + ------ + pretab.core.exceptions.InvalidParamError + If ``input_features`` is given but does not have one entry per input + feature. ``strict=False`` on the zip used to truncate silently, + returning a short name array for a wrong-length argument. + """ check_is_fitted(self, "n_features_in_") if input_features is None: input_features = [f"x{i}" for i in range(self.n_features_in_)] + elif len(input_features) != self.n_features_in_: + raise InvalidParamError( + f"input_features has {len(input_features)} entries, but " + f"{type(self).__name__} was fitted on {self.n_features_in_} features." + ) suffix = self._feature_suffix() names = [] for feature, n_cols in zip(input_features, self._output_sizes(), strict=False): diff --git a/pretab/core/locations.py b/pretab/core/locations.py index df22e23..1d44dd1 100644 --- a/pretab/core/locations.py +++ b/pretab/core/locations.py @@ -80,9 +80,25 @@ def resolve_locations( Sorted locations whose count lies in ``[min_count, max_count]`` (subject to the number of distinct candidates the supplement can provide). """ - locs = np.sort(np.asarray(locations, dtype=float)) - if dedupe: - locs = np.unique(locs) + locs = np.asarray(locations, dtype=float) + if importance is None: + locs = np.sort(locs) + if dedupe: + locs = np.unique(locs) + else: + # ``importance[i]`` describes ``locations[i]``, so it has to be carried + # through the same reordering. Sorting (or de-duplicating) ``locs`` on its + # own silently left the two misaligned and trimmed the wrong entries. + importance = np.asarray(importance) + if len(importance) != len(locs): + raise ValueError( + f"importance has {len(importance)} entries but locations has {len(locs)}." + ) + order = np.argsort(locs, kind="stable") + locs, importance = locs[order], importance[order] + if dedupe: + locs, first = np.unique(locs, return_index=True) + importance = importance[first] if len(locs) > max_count: locs = trim_to_count(locs, max_count, importance) if len(locs) < min_count and supplement is not None: diff --git a/pretab/transformers/embeddings/language_transformer.py b/pretab/transformers/embeddings/language_transformer.py index 797c345..9d0a4a6 100644 --- a/pretab/transformers/embeddings/language_transformer.py +++ b/pretab/transformers/embeddings/language_transformer.py @@ -73,7 +73,10 @@ def fit(self, X, y=None): self : object Fitted transformer. """ - self.n_features_in_ = X.shape[1] if len(X.shape) > 1 else 1 + # ``transform`` already normalizes via ``np.asarray``; do the same here so + # a plain list works (``X.shape`` raised ``AttributeError`` on one). + arr = np.asarray(X) + self.n_features_in_ = arr.shape[1] if arr.ndim > 1 else 1 self.model_ = self._resolve_model() return self diff --git a/tests/test_minor_defects.py b/tests/test_minor_defects.py new file mode 100644 index 0000000..75c19c4 --- /dev/null +++ b/tests/test_minor_defects.py @@ -0,0 +1,112 @@ +"""Regression tests for the small defects collected in issue #21. + +Each block is independent; they are grouped only because the fixes are small. +""" + +import numpy as np +import pytest + +from pretab.core.exceptions import InvalidParamError +from pretab.core.locations import resolve_locations +from pretab.transformers import BSplineTransformer + + +# --------------------------------------------------------------------------- # +# 1. resolve_locations must keep ``importance`` aligned with ``locations``. +# +# ``locs`` was sorted and de-duplicated while ``importance`` kept its original +# order, so the trim ranked the wrong entries. +# --------------------------------------------------------------------------- # +def test_resolve_locations_keeps_importance_aligned(): + locations = np.array([5.0, 1.0, 3.0]) + importance = np.array([0.1, 9.9, 0.2]) # 1.0 is by far the most important + + kept = resolve_locations(locations, min_count=1, max_count=1, importance=importance) + + np.testing.assert_allclose(kept, [1.0]) + + +def test_resolve_locations_importance_survives_dedupe(): + locations = np.array([5.0, 5.0, 1.0, 3.0]) + importance = np.array([0.1, 0.1, 9.9, 0.2]) + + kept = resolve_locations(locations, min_count=1, max_count=2, importance=importance) + + assert 1.0 in kept + + +def test_resolve_locations_rejects_mismatched_importance(): + with pytest.raises(ValueError, match="importance has"): + resolve_locations( + np.array([1.0, 2.0, 3.0]), min_count=1, max_count=1, importance=np.array([1.0]) + ) + + +def test_resolve_locations_without_importance_is_unchanged(): + locations = np.array([5.0, 1.0, 3.0, 3.0]) + + kept = resolve_locations(locations, min_count=1, max_count=3) + + np.testing.assert_allclose(kept, [1.0, 3.0, 5.0]) + + +# --------------------------------------------------------------------------- # +# 2. get_feature_names_out must reject a wrong-length ``input_features``. +# +# ``zip(..., strict=False)`` truncated silently, returning a short array. +# --------------------------------------------------------------------------- # +def test_feature_names_out_rejects_too_few_input_features(): + transformer = BSplineTransformer(output_dim=6).fit(np.random.default_rng(0).random((20, 2))) + + with pytest.raises(InvalidParamError, match="input_features has 1 entries"): + transformer.get_feature_names_out(["only_one"]) + + +def test_feature_names_out_rejects_too_many_input_features(): + transformer = BSplineTransformer(output_dim=6).fit(np.random.default_rng(0).random((20, 1))) + + with pytest.raises(InvalidParamError, match="was fitted on 1 features"): + transformer.get_feature_names_out(["a", "b"]) + + +def test_feature_names_out_accepts_the_right_length(): + transformer = BSplineTransformer(output_dim=6).fit(np.random.default_rng(0).random((20, 2))) + + names = transformer.get_feature_names_out(["a", "b"]) + + assert len(names) == transformer.transform(np.random.default_rng(1).random((5, 2))).shape[1] + assert names[0].startswith("a_") + assert names[-1].startswith("b_") + + +# --------------------------------------------------------------------------- # +# 3. LanguageEmbeddingTransformer.fit must accept a plain list. +# +# ``X.shape[1]`` raised AttributeError on a list, so the documented example +# could not run (it is marked ``# doctest: +SKIP``, hiding the breakage). +# --------------------------------------------------------------------------- # +def test_language_embedding_fit_accepts_a_list(): + from pretab.transformers import LanguageEmbeddingTransformer + + class _DummyModel: + def encode(self, texts, convert_to_numpy=True): + return np.ones((len(texts), 4)) + + transformer = LanguageEmbeddingTransformer(model=_DummyModel()) + transformer.fit([["red"], ["blue"], ["green"]]) + + assert transformer.n_features_in_ == 1 + assert transformer.transform([["red"], ["blue"], ["green"]]).shape == (3, 4) + + +def test_language_embedding_fit_still_accepts_arrays(): + from pretab.transformers import LanguageEmbeddingTransformer + + class _DummyModel: + def encode(self, texts, convert_to_numpy=True): + return np.ones((len(texts), 2)) + + transformer = LanguageEmbeddingTransformer(model=_DummyModel()) + transformer.fit(np.array([["a", "x"], ["b", "y"]], dtype=object)) + + assert transformer.n_features_in_ == 2