feat: add a scikit-learn adapter behind an optional extra - #18
Merged
Conversation
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.
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.
python_som.sklearn.SOMEstimator, installed withpip install "python-som[sklearn]". Last feature PRfor 0.6.0.
Why this is a separate module rather than more methods on SOM
The plan originally claimed duck typing was sufficient and
BaseEstimatorunnecessary. Measuredagainst scikit-learn 1.9, that is false:
clonePipeline.fitPipeline.predictGridSearchCVcross_val_scoreSOMaloneSOMEstimatorSince 1.7 those paths call
get_tags()and need__sklearn_tags__; scikit-learn's own error says toinherit
BaseEstimator. Duck typing gets youfitand nothing else that matters.Defining
__sklearn_tags__by hand also works and was rejected: it couples to an internal thatchanged 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.tomlalready declares.Two tests hold the line: importing
python_somin a subprocess assertssklearnis absent fromsys.modules, and a second assertssklearn.pyis 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.pyby name rather than beingloosened.
Verified in a clean venv, both ways
Design points worth reviewing
Every constructor argument is stored unmodified.
clonerebuilds by passingget_params()backto
__init__and then requires the result to compare equal, so an__init__that validates, coercesor derives anything breaks cloning — which is what
GridSearchCVdoes to every candidate. All of thatis deferred to
fit.input_lenis not a constructor argument. scikit-learn infers the feature count fromX, andn_features_in_reports it. A caller who changes their feature count should not have to remember asecond number that must agree.
The estimator's
fitstarts over;SOM.fitcontinues. Opposite behaviours, both deliberate:GridSearchCVfits one cloned estimator on fold after fold, and weights carried between folds wouldleak one fold into the next and make every score meaningless. Both are pinned by tests.
The GridSearchCV test checks the sign of
scoreend to end — it asserts a search over{x: [2, 6], y: [2, 6]}picks6x6. Ifscorewere not negated, a search would prefer the worstmap and this is what would catch it.
One config change
mypy relaxes
disallow_subclassing_anyfor this module alone, because scikit-learn ships nopy.typedso
BaseEstimatorisAnyunder--strict. Narrowed to the one module rather than a blankettype: ignoreon 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 --strictclean,mkdocs build --strictexits 0, bandit clean,pip-auditclean, architecture profile--strictpasses.