Skip to content

feat: add a scikit-learn adapter behind an optional extra - #18

Merged
andremsouza merged 2 commits into
masterfrom
feat/sklearn-adapter
Jul 30, 2026
Merged

feat: add a scikit-learn adapter behind an optional extra#18
andremsouza merged 2 commits into
masterfrom
feat/sklearn-adapter

Conversation

@andremsouza

Copy link
Copy Markdown
Owner

python_som.sklearn.SOMEstimator, installed with pip install "python-som[sklearn]". Last feature PR
for 0.6.0.

Why this is a separate module rather than more methods on SOM

The plan originally claimed duck typing was sufficient and BaseEstimator unnecessary. Measured
against scikit-learn 1.9, that is false:

approach clone Pipeline.fit Pipeline.predict GridSearchCV cross_val_score
methods on SOM alone ok ok fail fail fail
SOMEstimator ok ok ok ok ok

Since 1.7 those paths call get_tags() and need __sklearn_tags__; scikit-learn's own error says to
inherit BaseEstimator. Duck typing gets you fit and nothing else that matters.

Defining __sklearn_tags__ by hand also works and was rejected: it couples to an internal that
changed shape between 1.6 and 1.7, and scikit-learn explicitly says it does not recommend the approach.

The core stays numpy-only

An adapter may depend on the thing it adapts. That is what makes it an adapter rather than a
dependency, and it is the shape architecture-profile.toml already declares.

Two tests hold the line: importing python_som in a subprocess asserts sklearn is absent from
sys.modules, and a second asserts sklearn.py is the only module in the package that imports it —
so a second import would need a deliberate edit to the test.

The boundary test that scans for banned imports now exempts sklearn.py by name rather than being
loosened.

Verified in a clean venv, both ways

numpy only:    core trains, predict (20,), score -0.6803
               importing python_som.sklearn raises:
               "needs scikit-learn ... pip install "python-som[sklearn]" ...
                The rest of python_som does not need it"
after [sklearn]: SOMEstimator(x=4, y=4).fit(X).labels_ -> (30,)

Design points worth reviewing

Every constructor argument is stored unmodified. clone rebuilds by passing get_params() back
to __init__ and then requires the result to compare equal, so an __init__ that validates, coerces
or derives anything breaks cloning — which is what GridSearchCV does to every candidate. All of that
is deferred to fit.

input_len is not a constructor argument. scikit-learn infers the feature count from X, and
n_features_in_ reports it. A caller who changes their feature count should not have to remember a
second number that must agree.

The estimator's fit starts over; SOM.fit continues. Opposite behaviours, both deliberate:
GridSearchCV fits one cloned estimator on fold after fold, and weights carried between folds would
leak one fold into the next and make every score meaningless. Both are pinned by tests.

The GridSearchCV test checks the sign of score end to end — it asserts a search over
{x: [2, 6], y: [2, 6]} picks 6x6. If score were not negated, a search would prefer the worst
map and this is what would catch it.

One config change

mypy relaxes disallow_subclassing_any for this module alone, because scikit-learn ships no py.typed
so BaseEstimator is Any under --strict. Narrowed to the one module rather than a blanket
type: ignore on the class, which would have hidden any other subclassing mistake in the same file.

Verification

424 tests (17 new), 100% coverage, ruff / ruff format / mypy --strict clean, mkdocs build --strict exits 0, bandit clean, pip-audit clean, architecture profile --strict passes.

python_som.sklearn.SOMEstimator inherits BaseEstimator, ClusterMixin and TransformerMixin, and is
installed with `pip install "python-som[sklearn]"`.

The methods added to SOM in the previous change are enough when the caller invokes them directly. They
are not enough when scikit-learn does the calling. Measured against 1.9: pure duck typing gives clone
and Pipeline.fit, then fails Pipeline.predict, GridSearchCV and cross_val_score, all with
AttributeError, because since 1.7 those paths call get_tags() and need __sklearn_tags__. scikit-learn's
own error says to inherit BaseEstimator.

Defining __sklearn_tags__ by hand also works and was rejected: it couples to an internal that changed
shape between 1.6 and 1.7, and scikit-learn explicitly discourages it.

So the integration is an adapter in its own module. An adapter may depend on the thing it adapts, which
is what keeps the core numpy-only: importing python_som pulls in nothing but NumPy, asserted in a
subprocess, and a second test asserts sklearn.py is the only module in the package that imports it.

Every constructor argument is stored unmodified. clone rebuilds an estimator by passing get_params()
back to __init__ and then requires the result to compare equal, so an __init__ that validates or
derives anything breaks cloning. All of that is deferred to fit.

input_len is not a constructor argument, since scikit-learn infers the feature count from X.

The estimator's fit starts over rather than continuing, the opposite of SOM.fit. GridSearchCV fits one
cloned estimator on fold after fold, and weights carried between folds would leak one fold into the
next and make the scores meaningless. Both behaviours are pinned by tests so the difference stays a
decision.

The boundary test that scans for banned imports now exempts sklearn.py by name, and a new test asserts
the exemption covers exactly one module, so a second scikit-learn import would need a deliberate edit.

mypy relaxes disallow_subclassing_any for this module alone: scikit-learn ships no py.typed, so
BaseEstimator is Any. A blanket type: ignore on the class would have hidden any other subclassing
mistake in the same file.

Verified in a clean venv: with numpy alone the core trains and predicts, and importing the adapter
raises an ImportError naming the extra to install; after installing the extra it works.
@andremsouza
andremsouza merged commit 2d0bdd2 into master Jul 30, 2026
8 checks passed
@andremsouza
andremsouza deleted the feat/sklearn-adapter branch July 31, 2026 12:30
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.

1 participant