fix: correct three small defects from the audit - #32
Open
ChrisW09 wants to merge 1 commit into
Open
Conversation
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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_locationsmisalignedimportancewithlocationslocswas sorted and de-duplicated whileimportancekept its original order, so indexiof one no longer described entry
iof the other and the trim ranked the wrong entries:Now
importanceis carried through the same permutation (and the same dedupe), and a lengthmismatch raises instead of silently misbehaving.
This path is currently unreachable — nothing in the package calls
resolve_locations, itis only re-exported from
pretab/core/__init__.py— so this is a latent trap rather than alive bug. It is public API, though, so it seemed worth closing off.
2.
get_feature_names_outsilently truncated a wrong-lengthinput_featuresBasePreTabTransformer.get_feature_names_outusedzip(..., strict=False), so passing toofew names returned a short array rather than raising. Now validated against
n_features_in_, raisingInvalidParamError— matching scikit-learn's convention ofrejecting mismatched
input_features.3.
LanguageEmbeddingTransformer.fitrejected list inputfitcalledX.shape[1], which raisesAttributeErroron a plain list — including the listin the class's own docstring example, which is masked by
# doctest: +SKIPso CI nevercaught it.
transformalready normalizes withnp.asarray;fitnow 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 (includingthe dedupe path, the mismatch guard, and a no-importance regression check), three for the
input_featuresvalidation, two for list/array input to the embedding transformer (using astub model, so no
sentence-transformersdependency).Full suite: 489 passed, 9 xfailed.
ruff checkclean on changed files. pyright 71 → 72,entirely the
import pytestartifact that every test file produces in this checkout (pyrightis configured for a
.venvthat doesn't exist); no new diagnostic inpretab/.🤖 Generated with Claude Code