From e2deb3abd3bca202e1e41e008d8db455adf0378a Mon Sep 17 00:00:00 2001 From: Nakul Upadhya Date: Fri, 2 Oct 2026 21:56:08 -0400 Subject: [PATCH 1/3] Reject int feature_dict keys that collide with auto-filled columns Fixes #81. _feature_dict_to_features stored user groups and auto-filled singleton columns in one dict keyed by logical name, so an int key equal to the index of a column no group lists was overwritten by that column's singleton (configure_feature_dict(2, {0: [1]}) -> [[0]]), silently dropping the group from training. Now raise a ValueError naming the clashing keys. str keys and non-colliding int keys are unchanged. Existing tests missed it because every int-key case listed the key's own column, and several regression tests compare against configure_feature_dict itself. Added collision tests (configure and fit) and an every-column-exactly-once invariant test. Co-Authored-By: Claude Opus 5.5 --- sgtlearn/_features.py | 22 +++++++++++++++++----- tests/test_features.py | 29 +++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 5 deletions(-) diff --git a/sgtlearn/_features.py b/sgtlearn/_features.py index 2763551..7b1d706 100644 --- a/sgtlearn/_features.py +++ b/sgtlearn/_features.py @@ -81,9 +81,20 @@ def _feature_dict_to_features( if len(all_idxs) != len(set(all_idxs)): raise ValueError("Feature indices must be unique") - for i in range(n_features): - if i not in all_idxs: - index_dict[i] = [i] + listed = set(all_idxs) + unlisted = [i for i in range(n_features) if i not in listed] + # An int key equal to an unlisted column's index would be overwritten by + # that column's auto-filled singleton, silently dropping the group (#81). + clashes = [i for i in unlisted if i in index_dict] + if clashes: + raise ValueError( + f"feature_dict key(s) {clashes} equal the index of a column that no " + "group lists, so they would collide with that column's auto-filled " + "feature; use a different key (e.g. a str name) or list the column " + "in a group" + ) + for i in unlisted: + index_dict[i] = [i] out: list[FeatureInfoDict] = [] logical_names: list[str] = [] @@ -117,8 +128,9 @@ def configure_feature_dict( (``int``) or column names (``str``) when ``column_names`` or a pandas ``DataFrame`` was used for training. A group with more than one column is categorical; singletons are continuous. Unmentioned columns are - filled in as continuous singletons. When omitted, each column is its - own continuous feature. + filled in as continuous singletons keyed by their index, so an ``int`` + key must not equal the index of an unmentioned column (``ValueError``). + When omitted, each column is its own continuous feature. column_names Names of columns in ``X``, used to resolve string column references in ``feature_dict``. diff --git a/tests/test_features.py b/tests/test_features.py index 4cc2263..2cfc050 100644 --- a/tests/test_features.py +++ b/tests/test_features.py @@ -42,6 +42,35 @@ def test_configure_feature_dict_rejects_duplicate_indices() -> None: configure_feature_dict(4, feature_dict={0: [0], 1: [0]}) +@pytest.mark.parametrize("feature_dict", [{0: [2, 3]}, {5: [0, 1, 2]}]) +def test_configure_feature_dict_rejects_int_key_colliding_with_unlisted_column( + feature_dict: FeatureDict, +) -> None: + # Regression (#81): the auto-filled column used to overwrite the user's group. + with pytest.raises(ValueError, match="collide"): + configure_feature_dict(6, feature_dict) + + +def test_fit_rejects_int_key_colliding_with_unlisted_column() -> None: + X, y = make_classification(n_samples=60, n_features=6, random_state=0) + with pytest.raises(ValueError, match="collide"): + SGTClassifier(tao_n_runs=0).fit(X, y, feature_dict={0: [2, 3]}) + + +@pytest.mark.parametrize( + "feature_dict", + [{9: [2, 3]}, {0: [0, 1]}, {"0": [2, 3]}, {"a": [0, 5], 3: [2, 3]}, {7: [4]}], +) +def test_configure_feature_dict_covers_every_column_exactly_once( + feature_dict: FeatureDict, +) -> None: + pf = configure_feature_dict(6, feature_dict) + cols = sorted(c for f in pf.features for c in f["indices"]) + assert cols == list(range(6)) + groups = [f["indices"] for f in pf.features] + assert all(list(cols) in groups for cols in feature_dict.values()) + + @pytest.mark.parametrize( ("feature_dict", "column_names", "match"), [ From a470a39186ff18565fccd4607b0f1eddb25124b7 Mon Sep 17 00:00:00 2001 From: Nakul Upadhya Date: Fri, 2 Oct 2026 22:13:25 -0400 Subject: [PATCH 2/3] Add blind regression tests for _feature_dict_to_features test-checker bugfix run r-20261002-215910-e7c6 (53a5c14 -> e2deb3a): 23 tests written blind from the spec; 2 fail on the buggy revision and pass on the fix, 21 pass on both, none misguided. Unused numpy import removed for ruff. Co-Authored-By: Claude Opus 5.5 --- .test-checker/ledger.jsonl | 18 ++ .../reports/r-20261002-215910-e7c6.json | 46 +++++ .../reports/r-20261002-215910-e7c6.md | 41 ++++ ...features_py__feature_dict_to_features_1.py | 176 ++++++++++++++++++ 4 files changed, 281 insertions(+) create mode 100644 .test-checker/reports/r-20261002-215910-e7c6.json create mode 100644 .test-checker/reports/r-20261002-215910-e7c6.md create mode 100644 tests/regression/test_sgtlearn_features_py__feature_dict_to_features_1.py diff --git a/.test-checker/ledger.jsonl b/.test-checker/ledger.jsonl index f3f5ddd..716d57d 100644 --- a/.test-checker/ledger.jsonl +++ b/.test-checker/ledger.jsonl @@ -2079,3 +2079,21 @@ {"ts":"2026-10-02T20:30:43.618Z","run":"r-20261002-162723-b8b5","type":"classified","exec":1,"counts":{"sgtlearn-ensemble-random-sgforest-regressor-py--randomsgforestregressor-check-x-y":{"accepted":47}},"queued":0} {"ts":"2026-10-02T20:30:43.673Z","run":"r-20261002-162723-b8b5","type":"run_reported","likely_bugs":0,"needs_decision":0} {"ts":"2026-10-02T20:30:43.740Z","run":"r-20261002-162723-b8b5","type":"promoted","files":["tests/regression/test_sgtlearn_ensemble_random_sgforest_regressor_py__randomsgforestregressor_check_x_y_1.py"],"verified":["sgtlearn/ensemble/random_sgforest_regressor.py"]} +{"ts":"2026-10-03T01:56:55.420Z","run":"r-20261002-215655-a64c","type":"run_started","mode":"bugfix","revisions":{"buggy":"53a5c148d71076030fca81ed23bf8df64780e3d2","fixed":"e2deb3abd3bca202e1e41e008d8db455adf0378a"},"given":{"buggy":"HEAD~1","fixed":"HEAD"}} +{"ts":"2026-10-03T01:56:58.477Z","run":"r-20261002-215655-a64c","type":"target_added","target":"sgtlearn-features-py--feature-dict-to-features","file":"sgtlearn/_features.py","symbol":"_feature_dict_to_features","lines":[67,110],"rev":"fixed","body_sha":"29dc5fb2b59365161dee2e30347b09623388c1b3bb754f4d23d2b54cc95121e2"} +{"ts":"2026-10-03T01:57:15.773Z","run":"r-20261002-215655-a64c","type":"context_set","target":"sgtlearn-features-py--feature-dict-to-features","shingle_ratio":0} +{"ts":"2026-10-03T01:57:15.827Z","run":"r-20261002-215655-a64c","type":"payload_emitted","payload":"p-344a6d25","role":"spec","sha256":"45c0a4372d525e72ba232228bd996a02a80f51be816d5193576977d2afaa126f","target":"sgtlearn-features-py--feature-dict-to-features"} +{"ts":"2026-10-03T01:58:34.269Z","run":"r-20261002-215655-a64c","type":"spec_saved","target":"sgtlearn-features-py--feature-dict-to-features","shingle_ratio":0.049} +{"ts":"2026-10-03T01:58:34.323Z","run":"r-20261002-215655-a64c","type":"bundle_frozen","target":"sgtlearn-features-py--feature-dict-to-features","bundle":"b-a64c-sgtlearn-features-py--feature-dict-to-features"} +{"ts":"2026-10-03T01:58:37.621Z","run":"r-20261002-215655-a64c","type":"payload_emitted","payload":"p-8deb8c0c","role":"writer","sha256":"26903dea080139c423949477eeff1083f8611e8f39581e7ea8c71ce1bf4e3ec5","target":"sgtlearn-features-py--feature-dict-to-features"} +{"ts":"2026-10-03T01:58:55.491Z","type":"hook_blocked","event":"handoff-guard","reason":"internal error: unknown payload p-8deb8c0c"} +{"ts":"2026-10-03T01:59:10.243Z","run":"r-20261002-215910-e7c6","type":"run_started","mode":"bugfix","revisions":{"buggy":"53a5c148d71076030fca81ed23bf8df64780e3d2","fixed":"e2deb3abd3bca202e1e41e008d8db455adf0378a"},"given":{"buggy":"53a5c14","fixed":"e2deb3a"}} +{"ts":"2026-10-03T01:59:10.322Z","run":"r-20261002-215910-e7c6","type":"target_added","target":"sgtlearn-features-py--feature-dict-to-features","file":"sgtlearn/_features.py","symbol":"_feature_dict_to_features","lines":[67,110],"rev":"e2deb3a","body_sha":"29dc5fb2b59365161dee2e30347b09623388c1b3bb754f4d23d2b54cc95121e2"} +{"ts":"2026-10-03T01:59:22.638Z","run":"r-20261002-215910-e7c6","type":"context_set","target":"sgtlearn-features-py--feature-dict-to-features","shingle_ratio":0} +{"ts":"2026-10-03T01:59:22.696Z","run":"r-20261002-215910-e7c6","type":"spec_saved","target":"sgtlearn-features-py--feature-dict-to-features","shingle_ratio":0.049} +{"ts":"2026-10-03T01:59:22.757Z","run":"r-20261002-215910-e7c6","type":"bundle_frozen","target":"sgtlearn-features-py--feature-dict-to-features","bundle":"b-e7c6-sgtlearn-features-py--feature-dict-to-features"} +{"ts":"2026-10-03T01:59:22.820Z","run":"r-20261002-215910-e7c6","type":"payload_emitted","payload":"p-83941a5c","role":"writer","sha256":"c34e4e92e897e915e7377f88cb87523f573610e80f21906f117a39db56f97abf","target":"sgtlearn-features-py--feature-dict-to-features"} +{"ts":"2026-10-03T02:00:11.337Z","run":"r-20261002-215910-e7c6","type":"tests_submitted","target":"sgtlearn-features-py--feature-dict-to-features","round":0,"role":"writer","payload":"p-83941a5c","files":["test_sgtlearn_features_py__feature_dict_to_features_1.py"]} +{"ts":"2026-10-03T02:01:56.427Z","run":"r-20261002-215910-e7c6","type":"exec_completed","exec":1,"labels":{"buggy":{"pass":21,"failure":2,"load_errors":0},"fixed":{"pass":23,"load_errors":0}}} +{"ts":"2026-10-03T02:01:59.160Z","run":"r-20261002-215910-e7c6","type":"classified","exec":1,"counts":{"sgtlearn-features-py--feature-dict-to-features":{"neutral":21,"effective":2}},"queued":0} +{"ts":"2026-10-03T02:02:21.121Z","run":"r-20261002-215910-e7c6","type":"run_reported","likely_bugs":0,"needs_decision":0} diff --git a/.test-checker/reports/r-20261002-215910-e7c6.json b/.test-checker/reports/r-20261002-215910-e7c6.json new file mode 100644 index 0000000..e158238 --- /dev/null +++ b/.test-checker/reports/r-20261002-215910-e7c6.json @@ -0,0 +1,46 @@ +{ + "run": "r-20261002-215910-e7c6", + "mode": "bugfix", + "revisions": { + "buggy": { + "given": "53a5c14", + "commit": "53a5c148d71076030fca81ed23bf8df64780e3d2" + }, + "fixed": { + "given": "e2deb3a", + "commit": "e2deb3abd3bca202e1e41e008d8db455adf0378a" + } + }, + "date": "2026-10-03T02:02:21.120Z", + "harness": "claude-code", + "blind_isolation": "strong", + "summary": [ + { + "target": "sgtlearn-features-py--feature-dict-to-features", + "effective": 2, + "misguided": 0, + "neutral": 21, + "broken": 0, + "flaky": 0, + "dropped": 0 + } + ], + "likely_bugs": [], + "suspicions": [ + { + "target": "sgtlearn-features-py--feature-dict-to-features", + "analysis": "Logical mistakes: none found for the stated intent. The collision check runs after the uniqueness check and before auto-filling, so a user group is never overwritten; an int key equal to a column some group does list is not a collision. (Int keys sort by their string form, so 10 before 9; this is documented, intended behaviour and out of scope.)\nRobustness omissions: a str key such as \"3\" plus an unlisted column 3 yields duplicate logical names (out of scope per intent: str keys behave as before). Empty groups are accepted. bool keys are not validated. Invalid column references rely on the column-resolution helper to raise." + } + ], + "needs_decision": [], + "misguided": [], + "dropped": [], + "promotable": [ + { + "file": "tests/tcheck/test_sgtlearn_features_py__feature_dict_to_features_1.py", + "dest": "tests/regression/test_sgtlearn_features_py__feature_dict_to_features_1.py", + "tests": 23 + } + ], + "not_promotable": [] +} diff --git a/.test-checker/reports/r-20261002-215910-e7c6.md b/.test-checker/reports/r-20261002-215910-e7c6.md new file mode 100644 index 0000000..317c458 --- /dev/null +++ b/.test-checker/reports/r-20261002-215910-e7c6.md @@ -0,0 +1,41 @@ +# test-checker report r-20261002-215910-e7c6 + +- Mode: bugfix +- buggy: 53a5c14 → `53a5c148d7` +- fixed: e2deb3a → `e2deb3abd3` +- Date: 2026-10-03T02:02:21.120Z +- Harness: claude-code +- Blind isolation: strong + +## Summary + +| target | effective | misguided | neutral | broken | flaky | dropped | +|---|---|---|---|---|---|---| +| sgtlearn-features-py--feature-dict-to-features | 2 | 0 | 21 | 0 | 0 | 0 | + +## Likely bugs + +_none_ + +## Suspicions + +### sgtlearn-features-py--feature-dict-to-features + +Logical mistakes: none found for the stated intent. The collision check runs after the uniqueness check and before auto-filling, so a user group is never overwritten; an int key equal to a column some group does list is not a collision. (Int keys sort by their string form, so 10 before 9; this is documented, intended behaviour and out of scope.) +Robustness omissions: a str key such as "3" plus an unlisted column 3 yields duplicate logical names (out of scope per intent: str keys behave as before). Empty groups are accepted. bool keys are not validated. Invalid column references rely on the column-resolution helper to raise. + +## Needs your decision + +_none_ + +## Misguided tests and what happened to them + +_none_ + +## Dropped + +_none_ + +## Promotable tests + +- tests/tcheck/test_sgtlearn_features_py__feature_dict_to_features_1.py → `tests/regression/test_sgtlearn_features_py__feature_dict_to_features_1.py` (23 test(s)) diff --git a/tests/regression/test_sgtlearn_features_py__feature_dict_to_features_1.py b/tests/regression/test_sgtlearn_features_py__feature_dict_to_features_1.py new file mode 100644 index 0000000..3de2de4 --- /dev/null +++ b/tests/regression/test_sgtlearn_features_py__feature_dict_to_features_1.py @@ -0,0 +1,176 @@ +import copy + +import pytest + +import sgtlearn._features as m + + +def _simplify(features): + return [(list(f["indices"]), f["type"]) for f in features] + + +def test_empty_dict_gives_continuous_singletons() -> None: + features, names = m._feature_dict_to_features(3, {}) + assert _simplify(features) == [ + ([0], "continuous"), + ([1], "continuous"), + ([2], "continuous"), + ] + assert names == ("0", "1", "2") + + +def test_empty_dict_orders_by_string_key() -> None: + features, names = m._feature_dict_to_features(12, {}) + expected = sorted(range(12), key=str) + assert names == tuple(str(i) for i in expected) + assert [f["indices"] for f in features] == [[i] for i in expected] + + +def test_return_types() -> None: + features, names = m._feature_dict_to_features(3, {"g": [0, 1]}) + assert isinstance(features, list) + assert isinstance(names, tuple) + assert all(isinstance(n, str) for n in names) + assert all(isinstance(f, dict) for f in features) + for f in features: + assert all(type(i) is int for i in f["indices"]) + assert len(features) == len(names) + + +def test_multi_column_group_is_categorical_and_unlisted_filled() -> None: + features, names = m._feature_dict_to_features(3, {0: [0, 1]}) + assert names == ("0", "2") + assert _simplify(features) == [([0, 1], "categorical"), ([2], "continuous")] + + +def test_single_column_group_is_continuous() -> None: + features, names = m._feature_dict_to_features(2, {"g": [1]}) + assert names == ("0", "g") + assert _simplify(features) == [([0], "continuous"), ([1], "continuous")] + + +def test_str_keys_come_after_int_keys() -> None: + features, names = m._feature_dict_to_features(4, {"a": [1, 2]}) + assert names == ("0", "3", "a") + assert _simplify(features) == [ + ([0], "continuous"), + ([3], "continuous"), + ([1, 2], "categorical"), + ] + + +def test_str_keys_sorted_lexicographically() -> None: + features, names = m._feature_dict_to_features(4, {"b": [0, 1], "a": [2, 3]}) + assert names == ("a", "b") + assert _simplify(features) == [([2, 3], "categorical"), ([0, 1], "categorical")] + + +def test_indices_keep_user_order() -> None: + features, names = m._feature_dict_to_features(3, {"g": [2, 0, 1]}) + assert names == ("g",) + assert features[0]["indices"] == [2, 0, 1] + assert features[0]["type"] == "categorical" + + +def test_int_keys_ordered_by_string_ten_before_nine() -> None: + features, names = m._feature_dict_to_features(11, {10: [9, 10]}) + expected = tuple(sorted([str(i) for i in range(9)] + ["10"])) + assert names == expected + assert names.index("10") < names.index("8") + idx = names.index("10") + assert features[idx]["indices"] == [9, 10] + assert features[idx]["type"] == "categorical" + + +def test_user_and_auto_int_keys_interleave_by_string() -> None: + features, names = m._feature_dict_to_features(4, {3: [2, 3]}) + assert names == ("0", "1", "3") + assert _simplify(features) == [ + ([0], "continuous"), + ([1], "continuous"), + ([2, 3], "categorical"), + ] + + +def test_int_key_out_of_range_never_collides() -> None: + features, names = m._feature_dict_to_features(3, {5: [0, 1]}) + assert names == ("2", "5") + assert _simplify(features) == [([2], "continuous"), ([0, 1], "categorical")] + + +def test_int_key_matching_listed_column_is_allowed() -> None: + features, names = m._feature_dict_to_features(3, {1: [1, 2]}) + assert names == ("0", "1") + assert _simplify(features) == [([0], "continuous"), ([1, 2], "categorical")] + + +def test_int_key_matching_unlisted_column_raises_collide() -> None: + with pytest.raises(ValueError, match="collide") as exc: + m._feature_dict_to_features(3, {1: [0, 2]}) + assert "1" in str(exc.value) + + +def test_multiple_colliding_keys_listed_in_message() -> None: + with pytest.raises(ValueError, match="collide") as exc: + m._feature_dict_to_features(6, {3: [0, 1], 4: [2, 5]}) + msg = str(exc.value) + assert "3" in msg + assert "4" in msg + + +def test_str_key_equal_to_auto_name_does_not_collide() -> None: + features, names = m._feature_dict_to_features(3, {"0": [1, 2]}) + assert names == ("0", "0") + assert _simplify(features) == [([0], "continuous"), ([1, 2], "categorical")] + + +def test_duplicate_index_within_group_raises() -> None: + with pytest.raises(ValueError, match="Feature indices must be unique"): + m._feature_dict_to_features(3, {"g": [0, 0]}) + + +def test_duplicate_index_across_groups_raises() -> None: + with pytest.raises(ValueError, match="Feature indices must be unique"): + m._feature_dict_to_features(3, {"a": [0, 1], "b": [1, 2]}) + + +def test_duplicate_via_name_and_index_raises() -> None: + with pytest.raises(ValueError, match="Feature indices must be unique"): + m._feature_dict_to_features(2, {"g": ["a", 0]}, column_names=["a", "b"]) + + +def test_column_names_resolve_to_indices() -> None: + features, names = m._feature_dict_to_features( + 3, {"g": ["c", "a"]}, column_names=["a", "b", "c"] + ) + assert names == ("1", "g") + assert _simplify(features) == [([1], "continuous"), ([2, 0], "categorical")] + + +def test_out_of_range_index_error_propagates() -> None: + with pytest.raises(Exception) as ref: + m._resolve_column_index(5, 3, None) + with pytest.raises(type(ref.value)): + m._feature_dict_to_features(3, {"g": [0, 5]}) + + +def test_unknown_name_error_propagates() -> None: + cols = ["a", "b", "c"] + with pytest.raises(Exception) as ref: + m._resolve_column_index("zz", 3, cols) + with pytest.raises(type(ref.value)): + m._feature_dict_to_features(3, {"g": ["a", "zz"]}, column_names=cols) + + +def test_input_mapping_not_modified() -> None: + fd = {"g": [2, 0], 5: [1, 3]} + snapshot = copy.deepcopy(fd) + m._feature_dict_to_features(5, fd) + assert fd == snapshot + + +def test_every_column_appears_exactly_once() -> None: + features, names = m._feature_dict_to_features(7, {"x": [6, 1], 0: [0, 3]}) + all_idx = [i for f in features for i in f["indices"]] + assert sorted(all_idx) == list(range(7)) + assert names == ("0", "2", "4", "5", "x") From fe1b46018790be7a3907f45cfc564fe84432e9c8 Mon Sep 17 00:00:00 2001 From: Nakul Upadhya Date: Fri, 2 Oct 2026 22:21:55 -0400 Subject: [PATCH 3/3] Rename auto-filled columns instead of rejecting clashing keys Auto-filled column i is named str(i) unless a feature_dict key already uses that name, in which case it becomes "_1" ("_2", ...). This replaces the ValueError from the previous commit: {0: [2, 3]} now keeps the user's group "0" and names column 0 "0_1". It also fixes the duplicate-name case for str keys ({"0": [1, 2]} gave ("0", "0")). Keys whose string forms coincide (0 and "0") now raise ValueError. Three blind regression tests asserted the old raise/duplicate behaviour; updated per the maintainer's decision on the new naming rule. Co-Authored-By: Claude Opus 5.5 --- sgtlearn/_features.py | 51 ++++++++++------- ...features_py__feature_dict_to_features_1.py | 27 +++++---- tests/test_features.py | 57 +++++++++++++++---- 3 files changed, 91 insertions(+), 44 deletions(-) diff --git a/sgtlearn/_features.py b/sgtlearn/_features.py index 7b1d706..899d700 100644 --- a/sgtlearn/_features.py +++ b/sgtlearn/_features.py @@ -24,8 +24,9 @@ class ProcessedFeatures: ``estimator.feature_importances_[i]`` after a fit without TAO. logical_names Parallel names for ``features``. When ``feature_dict`` is supplied, - these are the stringified keys; omitted columns are filled as - ``\"0\"``, ``\"1\"``, …. Default (no ``feature_dict``) uses + these are the stringified keys; each omitted column ``i`` is named + ``str(i)``, or ``\"_1\"`` (``_2``, …) if a key already uses that + name. Default (no ``feature_dict``) uses ``\"0\"`` … ``\"n_features-1\"`` even if ``X`` is a pandas DataFrame (DataFrame column names are stored on ``feature_names_in_`` instead). """ @@ -81,32 +82,36 @@ def _feature_dict_to_features( if len(all_idxs) != len(set(all_idxs)): raise ValueError("Feature indices must be unique") + # (is str key, logical name, columns); int keys and auto-filled columns sort first. + entries = [(isinstance(k, str), str(k), cols) for k, cols in index_dict.items()] + taken = {name for _, name, _ in entries} + if len(taken) != len(entries): + raise ValueError("feature_dict keys must have distinct string forms") + + # Auto-fill unlisted columns as singletons named str(i). If a user key + # already has that name, suffix it ("0_1", "0_2", ...) so the user's group + # is never overwritten or shares its name (#81). listed = set(all_idxs) - unlisted = [i for i in range(n_features) if i not in listed] - # An int key equal to an unlisted column's index would be overwritten by - # that column's auto-filled singleton, silently dropping the group (#81). - clashes = [i for i in unlisted if i in index_dict] - if clashes: - raise ValueError( - f"feature_dict key(s) {clashes} equal the index of a column that no " - "group lists, so they would collide with that column's auto-filled " - "feature; use a different key (e.g. a str name) or list the column " - "in a group" - ) - for i in unlisted: - index_dict[i] = [i] + for i in range(n_features): + if i in listed: + continue + name, n = str(i), 0 + while name in taken: + n += 1 + name = f"{i}_{n}" + taken.add(name) + entries.append((False, name, [i])) out: list[FeatureInfoDict] = [] logical_names: list[str] = [] - for key in sorted(index_dict.keys(), key=lambda k: (isinstance(k, str), str(k))): - cols = list(index_dict[key]) + for _, name, cols in sorted(entries, key=lambda e: (e[0], e[1])): out.append( { "type": "categorical" if len(cols) > 1 else "continuous", - "indices": cols, + "indices": list(cols), } ) - logical_names.append(str(key)) + logical_names.append(name) return out, tuple(logical_names) @@ -128,9 +133,11 @@ def configure_feature_dict( (``int``) or column names (``str``) when ``column_names`` or a pandas ``DataFrame`` was used for training. A group with more than one column is categorical; singletons are continuous. Unmentioned columns are - filled in as continuous singletons keyed by their index, so an ``int`` - key must not equal the index of an unmentioned column (``ValueError``). - When omitted, each column is its own continuous feature. + filled in as continuous singletons named ``str(i)``; if a key already + uses that name, the column is named ``"_1"`` (then ``_2``, …) + instead. Keys must have distinct string forms (``0`` and ``"0"`` + together raise ``ValueError``). When omitted, each column is its own + continuous feature. column_names Names of columns in ``X``, used to resolve string column references in ``feature_dict``. diff --git a/tests/regression/test_sgtlearn_features_py__feature_dict_to_features_1.py b/tests/regression/test_sgtlearn_features_py__feature_dict_to_features_1.py index 3de2de4..c10ba66 100644 --- a/tests/regression/test_sgtlearn_features_py__feature_dict_to_features_1.py +++ b/tests/regression/test_sgtlearn_features_py__feature_dict_to_features_1.py @@ -104,23 +104,26 @@ def test_int_key_matching_listed_column_is_allowed() -> None: assert _simplify(features) == [([0], "continuous"), ([1, 2], "categorical")] -def test_int_key_matching_unlisted_column_raises_collide() -> None: - with pytest.raises(ValueError, match="collide") as exc: - m._feature_dict_to_features(3, {1: [0, 2]}) - assert "1" in str(exc.value) +def test_int_key_matching_unlisted_column_renames_auto_column() -> None: + features, names = m._feature_dict_to_features(3, {1: [0, 2]}) + assert names == ("1", "1_1") + assert _simplify(features) == [([0, 2], "categorical"), ([1], "continuous")] -def test_multiple_colliding_keys_listed_in_message() -> None: - with pytest.raises(ValueError, match="collide") as exc: - m._feature_dict_to_features(6, {3: [0, 1], 4: [2, 5]}) - msg = str(exc.value) - assert "3" in msg - assert "4" in msg +def test_multiple_colliding_keys_each_get_renamed_auto_column() -> None: + features, names = m._feature_dict_to_features(6, {3: [0, 1], 4: [2, 5]}) + assert names == ("3", "3_1", "4", "4_1") + assert _simplify(features) == [ + ([0, 1], "categorical"), + ([3], "continuous"), + ([2, 5], "categorical"), + ([4], "continuous"), + ] -def test_str_key_equal_to_auto_name_does_not_collide() -> None: +def test_str_key_equal_to_auto_name_renames_auto_column() -> None: features, names = m._feature_dict_to_features(3, {"0": [1, 2]}) - assert names == ("0", "0") + assert names == ("0_1", "0") assert _simplify(features) == [([0], "continuous"), ([1, 2], "categorical")] diff --git a/tests/test_features.py b/tests/test_features.py index 2cfc050..8ac5420 100644 --- a/tests/test_features.py +++ b/tests/test_features.py @@ -42,24 +42,60 @@ def test_configure_feature_dict_rejects_duplicate_indices() -> None: configure_feature_dict(4, feature_dict={0: [0], 1: [0]}) -@pytest.mark.parametrize("feature_dict", [{0: [2, 3]}, {5: [0, 1, 2]}]) -def test_configure_feature_dict_rejects_int_key_colliding_with_unlisted_column( - feature_dict: FeatureDict, +@pytest.mark.parametrize( + ("feature_dict", "expected"), + [ + # Regression (#81): auto-filled column 0 used to overwrite group 0. + ( + {0: [2, 3]}, + [("0", [2, 3]), ("0_1", [0]), ("1", [1]), ("4", [4]), ("5", [5])], + ), + ( + {"0": [2, 3]}, + [("0_1", [0]), ("1", [1]), ("4", [4]), ("5", [5]), ("0", [2, 3])], + ), + ( + {0: [2], "0_1": [3]}, + [ + ("0", [2]), + ("0_2", [0]), + ("1", [1]), + ("4", [4]), + ("5", [5]), + ("0_1", [3]), + ], + ), + ], +) +def test_configure_feature_dict_renames_auto_column_clashing_with_key( + feature_dict: FeatureDict, expected: list[tuple[str, list[int]]] ) -> None: - # Regression (#81): the auto-filled column used to overwrite the user's group. - with pytest.raises(ValueError, match="collide"): - configure_feature_dict(6, feature_dict) + pf = configure_feature_dict(6, feature_dict) + assert list(zip(pf.logical_names, [f["indices"] for f in pf.features])) == expected + + +def test_configure_feature_dict_rejects_keys_with_same_string_form() -> None: + with pytest.raises(ValueError, match="distinct"): + configure_feature_dict(4, {0: [1], "0": [2]}) -def test_fit_rejects_int_key_colliding_with_unlisted_column() -> None: +def test_fit_keeps_int_key_group_clashing_with_unlisted_column() -> None: X, y = make_classification(n_samples=60, n_features=6, random_state=0) - with pytest.raises(ValueError, match="collide"): - SGTClassifier(tao_n_runs=0).fit(X, y, feature_dict={0: [2, 3]}) + clf = SGTClassifier(tao_n_runs=0).fit(X, y, feature_dict={0: [2, 3]}) + assert [2, 3] in [f["indices"] for f in clf.processed_features_.features] @pytest.mark.parametrize( "feature_dict", - [{9: [2, 3]}, {0: [0, 1]}, {"0": [2, 3]}, {"a": [0, 5], 3: [2, 3]}, {7: [4]}], + [ + {9: [2, 3]}, + {0: [0, 1]}, + {"0": [2, 3]}, + {0: [2, 3]}, + {5: [0, 1, 2]}, + {"a": [0, 5], 3: [2, 3]}, + {7: [4]}, + ], ) def test_configure_feature_dict_covers_every_column_exactly_once( feature_dict: FeatureDict, @@ -69,6 +105,7 @@ def test_configure_feature_dict_covers_every_column_exactly_once( assert cols == list(range(6)) groups = [f["indices"] for f in pf.features] assert all(list(cols) in groups for cols in feature_dict.values()) + assert len(set(pf.logical_names)) == len(pf.logical_names) @pytest.mark.parametrize(