Repository navigation
Sklearn api compliance - #92
Conversation
- Update model state validation in stream() to raise Scikit-Learn's NotFittedError. - Track n_features_in_ and validate feature consistency in predict() and decision_function().- Change generated variable names to include _ suffix and add property decorators to ensure backward compatibility - Fix class inheritance MRO order by moving BaseEstimator to the end. - Enhance _convert_to_array() to reject empty arrays, complex types, and invalid numeric inputs with appropriate exceptions as scikit-learn demands. - Fortify _check_missing_values() to validate both NaN and infinite values, and update test assertions.
Add a _compat shim so scikit-learn remains an extras install, densify sparse inputs, keep init params immutable during fit, and finish the outlier API (score_samples, offset_, decision/predict consistency) so check_estimator passes. Co-authored-by: Cursor <cursoragent@cursor.com>
Accept scipy.sparse input by densifying at validation, fix stream() for sparse rows, raise a clear ImportError without scipy, and document the behavior plus a PyNomaly[sparse] extra. Co-authored-by: Cursor <cursoragent@cursor.com>
vc1492a
left a comment
vc1492a
left a comment
There was a problem hiding this comment.
Deep review done (manual + AI-assisted). Overall this is in good shape and ready to merge into the feature branch once the fit(X, y) comment below is addressed or consciously deferred.
Verified locally against head fa903ec:
- Full test suite: 46 passed, 4 skipped
check_estimator(LoOP()): all checks pass (one documented xfail:check_fit1d, since LoOP intentionally accepts 1-d input and reshapes to(-1, 1))- Param immutability:
get_params()identical before/afterfit()(effective values live onn_neighbors_,_fit_n_jobs, etc.) - Re-fit across input modes (raw data → distance matrix) produces correct, consistent scores
- Sparse CSR/CSC: fit/predict/stream work, dense/sparse scores identical, clear
ImportErrorwithout scipy predict/decision_function/score_samplesare mutually consistent (decision = scores - offset_, labels fromdecision >= 0)- Distance-matrix mode:
n_features_in_ = 1, predict works on single-feature input
Findings (line comments):
fit(X, y)with a(n, 1)label array silently becomesdistance_matrixand returns an unfitted model — the one thing I'd fix before mergefit()warns-and-returns on invalid input instead of raisingValueError(acceptable for BC, revisit in the deprecation-warnings PR)stream()return annotation saysnp.ndarraybut now returnsfloatdocs/api.mdstill shows legacy non-underscore attribute names as primary
On the read-side breaking change we discussed: confirmed distance_matrix after fit is still the init input (often None), and the computed matrix is only on distance_matrix_. That's correct per sklearn's config/state firewall — it just needs to be loud in the changelog and the follow-up warnings PR.
No automation will merge this — approving only records the review; merge stays manual.
| | Parameter | Type | Default | Description | | ||
| |---|---|---|---| | ||
| | `data` | `np.ndarray` or `pd.DataFrame` | `None` | Input data as a 2D array with shape (n_observations, n_features). Mutually exclusive with `distance_matrix`. | | ||
| | `data` | `np.ndarray`, `pd.DataFrame`, or `scipy.sparse` matrix | `None` | Input data as a 2D array with shape (n_observations, n_features). Sparse matrices are accepted and densified internally (requires scipy). Mutually exclusive with `distance_matrix`. | |
There was a problem hiding this comment.
Docs in this file still reference the legacy non-underscore attributes (clf.local_outlier_probabilities on line 50, the attribute table at lines 86/90). They still work as read-only property aliases, so nothing is wrong — but since fitted state now canonically lives on local_outlier_probabilities_, distance_matrix_, etc., the docs should show the underscore names as primary and mention the aliases as deprecated. Good candidate for the follow-up PR that adds warnings on legacy attribute access (note: distance_matrix after fit is the init input, not the computed matrix — that one has no alias by design and is the main read-side breaking change to document).
Align the annotation and API docs with the actual scalar return value, and note the type change in the 1.0.0 changelog. Co-authored-by: Cursor <cursoragent@cursor.com>
Restrict the deprecated positional distance_matrix fallback in fit() to calls with no raw data and a y wider than one column, so sklearn-style fit(X, y) with (n, 1) labels fits normally instead of returning an unfitted model. Adds a regression test. Co-authored-by: Cursor <cursoragent@cursor.com>
Move data from positional constructor calls into fit(), fix the distance-matrix self-removal NameError in README/user-guide, and note the positional constructor break in the 1.0.0 changelog. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@SaritNike finished my review and addressed two small issues I identified with the help of Fable. I also took some time to update our documentation and example scripts to reflect the changes in this PR. Please take another look. If this looks good to you, let's go ahead and merge in and address the deprecation warnings in a follow up. |
|
I took another round and reviewed it. we can merge it into the feature branch |
pandas is not a hard dependency, so bind the name only under typing.TYPE_CHECKING for the string type hints instead of importing it at runtime. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…lity. The exceptions moved to PyNomaly.exceptions in 1.0.0 and were no longer importable from PyNomaly.loop. Import them back into loop, list them in loop.__all__, add a test for the import path, and document the new home in the changelog and API docs. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…e job. Drop --exit-zero from the E9/F63/F7/F82 flake8 run so syntax errors and undefined names fail the build. The style/complexity run stays advisory. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ed_failed_checks. The keyword was added in scikit-learn 1.6, which needs Python 3.9+. On Python 3.8 (scikit-learn 1.3.x) the test raised TypeError and failed CI; it now skips with a clear reason there and runs everywhere else. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Remove 3.8 from the CI matrix, declare python_requires='>=3.9' with 3.9 through 3.14 classifiers in setup.py, update the supported-version statements in the README and docs, and record the removal under 1.0.0 in the changelog. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Every remaining CI Python version (3.9+) resolves scikit-learn 1.6.1 or newer, all of which support check_estimator(expected_failed_checks=...), so the compliance test now always runs. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
I see that you have added more commits. Feel free to merge when you are ready |
Translate SaritNike's contract test from the 0.4.0 constructor to the scikit-learn-style API: configuration-only constructor, data passed to fit(X, y=None, ...), _-suffixed fitted attributes with their read-only legacy aliases, the sklearn outlier methods, get_params() keys, the LoOP alias, and the exception classes now defined in PyNomaly.exceptions and re-exported from PyNomaly.loop. The structure and intent of the original test are kept; cluster_labels_u is dropped from the expected attributes because it became private (_cluster_labels_u) in 1.0.0. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Add public API consistency test
In the branch feat/sklearn-api-convention we have realized that quite a lot was actually still missing to be fully compliant with the sklearn API. This is why we have decided to add and test these new fixes first in a separate branch.
Now that the check-api-compliance test is successfully passing we are ready to merge it all into the feat/sklearn-api-convention branch