Skip to content

fix: correct three small defects from the audit - #32

Open
ChrisW09 wants to merge 1 commit into
mainfrom
fix/minor-defects-batch
Open

fix: correct three small defects from the audit#32
ChrisW09 wants to merge 1 commit into
mainfrom
fix/minor-defects-batch

Conversation

@ChrisW09

Copy link
Copy Markdown
Collaborator

Fixes #21.

Three independent small fixes, grouped because each is a few lines. Happy to split if you'd
rather review them separately.

1. resolve_locations misaligned importance with locations

locs was sorted and de-duplicated while importance kept its original order, so index i
of one no longer described entry i of the other and the trim ranked the wrong entries:

resolve_locations(np.array([5.0, 1.0, 3.0]), min_count=1, max_count=1,
                  importance=np.array([0.1, 9.9, 0.2]))
# [3.]   -- expected [1.], the high-importance location

Now importance is carried through the same permutation (and the same dedupe), and a length
mismatch raises instead of silently misbehaving.

This path is currently unreachable — nothing in the package calls resolve_locations, it
is only re-exported from pretab/core/__init__.py — so this is a latent trap rather than a
live bug. It is public API, though, so it seemed worth closing off.

2. get_feature_names_out silently truncated a wrong-length input_features

BasePreTabTransformer.get_feature_names_out used zip(..., strict=False), so passing too
few names returned a short array rather than raising. Now validated against
n_features_in_, raising InvalidParamError — matching scikit-learn's convention of
rejecting mismatched input_features.

3. LanguageEmbeddingTransformer.fit rejected list input

fit called X.shape[1], which raises AttributeError on a plain list — including the list
in the class's own docstring example, which is masked by # doctest: +SKIP so CI never
caught it. transform already normalizes with np.asarray; fit now does the same.

Not fixed: the doubled name prefix

The issue's fourth item — ndarray input producing num_feature_0__feature_0 — is left as is.
It's cosmetic, and the fix would mean changing the synthesized column stems, which changes
public output names for every ndarray user. That seemed disproportionate to the complaint;
happy to do it if you disagree.

Tests

New file tests/test_minor_defects.py, 9 tests: four for the importance alignment (including
the dedupe path, the mismatch guard, and a no-importance regression check), three for the
input_features validation, two for list/array input to the embedding transformer (using a
stub model, so no sentence-transformers dependency).

Full suite: 489 passed, 9 xfailed. ruff check clean on changed files. pyright 71 → 72,
entirely the import pytest artifact that every test file produces in this checkout (pyright
is configured for a .venv that doesn't exist); no new diagnostic in pretab/.

🤖 Generated with Claude Code

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) <noreply@anthropic.com>
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.

fix: assorted minor defects (importance misalignment, names, docs)

1 participant