MPCA pipeline - #18
Conversation
|
You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool. What Enabling Code Scanning Means:
For more information about GitHub Code Scanning, check out the documentation. |
There was a problem hiding this comment.
Pull request overview
This PR introduces an MPCA-based training pipeline and refactors the MPCA implementation and related APIs/docs to improve usability, scalability, and consistency across the library.
Changes:
- Add an
MPCATrainerpipeline (MPCA → feature selection → classifier) for end-to-end training. - Refactor
MPCAto support chunked fitting, new parameter names (explained_variance_ratio,output_shape), and updated fitted-attribute naming (*_). - Align neighbor/normalization parameter naming across utilities/estimators (
n_neighbors,k_neighbors,normalize) and update docs/tests/CI accordingly.
Reviewed changes
Copilot reviewed 26 out of 27 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| TUTORIALS.md | Adds worked tutorial examples and moves “quick start” style content out of README. |
| README.md | Restructures getting-started docs and links to new tutorials. |
| docs/source/usage.rst | Updates usage examples to match new variable naming (X) and API usage. |
| docs/source/tutorial.rst | Updates tutorial snippets to match updated examples/naming. |
| kalelinear/utils/_base.py | Renames lap_norm parameters to sklearn-style names (n_neighbors, normalize). |
| kalelinear/transformer/_tca.py | Updates lap_norm callsite to new keyword (n_neighbors). |
| kalelinear/transformer/_base.py | Adds ridge regularization for generalized eigenproblems; renames fitted attrs to X_fit_ etc. |
| kalelinear/transformer/_mpca.py | Major MPCA refactor: chunked stats, new params, new fitted attribute names. |
| kalelinear/pipeline/init.py | Adds (empty) pipeline package init file. |
| kalelinear/pipeline/mpca_trainer.py | Adds new MPCA→feature selection→classifier trainer implementation. |
| kalelinear/estimator/base.py | Stabilizes QP solving by projecting Hessian onto the PSD cone. |
| kalelinear/estimator/_manifold_learn.py | Renames k_neighbour to k_neighbors and updates lap_norm keyword usage. |
| kalelinear/estimator/_coir.py | Renames k_neighbour to k_neighbors and updates lap_norm keyword usage. |
| kalelinear/estimator/_artl.py | Renames k_neighbour to k_neighbors and updates lap_norm keyword usage. |
| kalelinear/estimator/_gsda.py | Cleans up variable naming (n_samples, X_tgt) and a comment. |
| tests/utils/test_utils.py | Updates lap_norm keyword args and renames local variables for consistency. |
| tests/transformer/test_transformer.py | Renames local variables (X, Xs, Xt) for consistency. |
| tests/transformer/test_mpca.py | Updates MPCA API usage (explained_variance_ratio, proj_mats_, output_shape_). |
| tests/transformer/test_mida.py | Renames local variables (X) for consistency. |
| tests/estimator/test_estimator.py | Renames local variables (X) for consistency. |
| .github/workflows/test.yml | Adds CI test workflow with coverage reporting. |
| .github/workflows/release.yml | Adds PyPI/TestPyPI release workflow. |
| .github/workflows/project.yml | Adds workflow to auto-assign issues/PRs to a GitHub Project. |
| .github/workflows/pre-commit.yml | Adds pre-commit workflow. |
| .github/workflows/codeql-analysis.yml | Adds CodeQL scanning workflow. |
| .github/workflows/changelog.yml | Adds changelog generation workflow. |
| .github/CHANGELOG.md | Adds initial changelog content. |
Suppressed comments (1)
kalelinear/transformer/_mpca.py:163
- The docstring example shows truncation to 50 components without setting
n_components=50, so the example output shape(40, 50)is not reproducible as written.
>>> X_projected = mpca.transform(X)
>>> X_projected.shape
(40, 50)
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 30 out of 31 changed files in this pull request and generated no new comments.
Suppressed comments (10)
kalelinear/pipeline/mpca_trainer.py:149
fit()mutatesself.grid_search.param_grid["C"]by appending a data-dependent value (1 / n_samples). Across repeated fits with different sample sizes, this grows and changes the search space, making results non-reproducible and progressively slower. Build a per-fit param_grid copy (including the extra value) instead of mutating the estimator's stored grid in place.
if self.auto_classifier_param:
extra_c = 1 / X.shape[0]
if extra_c not in self.grid_search.param_grid["C"]:
self.grid_search.param_grid["C"].append(extra_c)
self.grid_search.fit(X_transformed, y)
self.clf = self.grid_search.best_estimator_
kalelinear/estimator/_manifold_learn.py:31
- This estimator accepts
**kwargs, so legacy callers passingk_neighbour=will currently be silently ignored (captured inkwargs) rather than controllingk_neighbors. To avoid a hard-to-diagnose behavior change, consider translatingk_neighbour→k_neighborswith a deprecation warning.
def __init__(
self,
C=1.0,
kernel="linear",
gamma_=1.0,
solver="osqp",
k_neighbors=3,
manifold_metric="cosine",
knn_mode="distance",
**kwargs,
):
kalelinear/estimator/_coir.py:55
- Because this constructor has
**kwargs, legacy callers passingk_neighbour=will be accepted but silently ignored (captured inkwargs), changing behavior without warning. Consider translatingk_neighbour→k_neighborswith a deprecation warning (and error if both are provided).
def __init__(
self,
C=1.0,
kernel="linear",
lambda_=1.0,
mu=0.0,
k_neighbors=3,
manifold_metric="cosine",
knn_mode="distance",
solver="osqp",
covariate_encoder=None,
**kwargs,
):
kalelinear/estimator/_artl.py:115
- Since this constructor accepts
**kwargs, legacy callers passingk_neighbour=will now be accepted but ignored (captured inkwargs). Translatingk_neighbour→k_neighborswith a deprecation warning avoids a silent behavior change.
def __init__(
self,
C=1.0,
kernel="linear",
lambda_=1.0,
gamma_=0.0,
k_neighbors=5,
solver="osqp",
manifold_metric="cosine",
knn_mode="distance",
**kwargs,
):
kalelinear/pipeline/mpca_trainer.py:88
- When
mpca_paramsis None,self.mpca_paramsis set to the module-leveldefault_mpca_paramsdict by reference. This shares a mutable dict across allMPCATrainerinstances, so any in-place mutation (now or in the future) would leak between trainers. Prefer copying the defaults (and user-provided dicts) before storing them onself.
if mpca_params is None:
self.mpca_params = default_mpca_params
else:
self.mpca_params = mpca_params
self.mpca = MPCA(**self.mpca_params)
kalelinear/pipeline/mpca_trainer.py:142
fit()mutates the init parameterself.n_features(and usesself.feature_orderwithout an underscore). In scikit-learn-style estimators, init parameters should remain unchanged after construction; learned values should be stored as fitted attributes (e.g.n_features_,feature_order_) to keep cloning / parameter introspection predictable.
# feature selection
if self.n_features is None:
self.n_features = X_transformed.shape[1]
self.feature_order = self.mpca.idx_order_
else:
f_score, p_val = f_classif(X_transformed, y)
self.feature_order = (-1 * f_score).argsort()
X_transformed = X_transformed[:, self.feature_order][:, : self.n_features]
kalelinear/pipeline/mpca_trainer.py:205
- After switching to fitted attributes for feature selection (e.g.
feature_order_/n_features_),_extract_feature()should use those fitted attributes rather than the init parameters. Otherwise,n_features=Nonewill still lead to slicing withNone(or stale values) at prediction time.
check_is_fitted(self.clf)
X_transformed = self.mpca.transform(X)
return X_transformed[:, self.feature_order][:, : self.n_features]
kalelinear/utils/_base.py:12
- Renaming
n_neighbour→n_neighborsandnormalise→normalizeis a breaking change for a public utility (lap_normis exported fromkalelinear.utils). Consider accepting the legacy keyword arguments as deprecated aliases to avoid unexpectedly breaking downstream code.
def lap_norm(X, n_neighbors=3, metric="cosine", mode="distance", normalize=True):
"""[summary]
kalelinear/estimator/base.py:34
- With
**kwargsin the signature, callers using the legacy keywordk_neighbourwill not get aTypeErroranymore; the value will be silently ignored and stored inself.kwargs, changing behavior without warning. Explicitly mapk_neighbour→k_neighbors(and warn) or raise if both are provided.
def __init__(
self,
kernel="linear",
k_neighbors=5,
manifold_metric="cosine",
knn_mode="distance",
pos_label=1,
neg_label=-1,
**kwargs,
):
super().__init__()
self.kernel = kernel
self.k_neighbors = k_neighbors
self.manifold_metric = manifold_metric
self.knn_mode = knn_mode
tests/pipeline/test_mpca_trainer.py:27
- Variable naming in this new test uses
xfor the feature tensor while most other updated tests/docs useX(andXs/Xt) for feature matrices. Renaming here would make the naming consistency work in this PR apply uniformly.
def test_mpca_trainer(classifier, params, gait):
x = gait["fea3D"].transpose((3, 0, 1, 2))
x = x[:20, :]
y = gait["gnd"][:20].reshape(-1)
trainer = MPCATrainer(classifier=classifier, **params)
trainer.fit(x, y)
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 30 out of 31 changed files in this pull request and generated 1 comment.
Suppressed comments (4)
kalelinear/pipeline/mpca_trainer.py:147
- When
vectorize=True,MPCA.transform()already reorders columns byidx_order_. Re-applyingself.mpca.idx_order_here permutes the already-sorted features a second time, so training uses a different feature ordering than intended (and it also affects_extract_feature()which appliesfeature_order_again). Use an identity ordering whenn_features is None(or avoid reindexing entirely in that branch).
if self.n_features is None:
self.n_features_ = X_transformed.shape[1]
self.feature_order_ = self.mpca.idx_order_
else:
f_score, p_val = f_classif(X_transformed, y)
kalelinear/utils/_base.py:15
- Renaming
lap_normkeyword arguments (n_neighbour→n_neighbors,normalise→normalize) is a breaking change for external callers. Since this is a utility used across estimators/transformers, it’s safer to accept the old keywords as deprecated aliases (with a warning) so older code fails less abruptly.
def lap_norm(X, n_neighbors=3, metric="cosine", mode="distance", normalize=True):
"""[summary]
Parameters
----------
.github/workflows/test.yml:95
- The Codecov upload step requires
CODECOV_TOKEN, but repository secrets are not available to fork-based pull requests. As written, this step can fail PR CI for external contributors. Consider skipping the upload when the token is absent (or configuring Codecov OIDC / non-failing uploads).
- name: Report coverage
uses: codecov/codecov-action@v4
with:
token: ${{ secrets.CODECOV_TOKEN }}
kalelinear/pipeline/mpca_trainer.py:125
- This second
classifier_paramsvalidation/initialization block duplicates the logic already handled just above (and reassignsself.clf/auto_classifier_param). It increases maintenance risk without adding behavior. Since the first block already validates and sets upclf/grid_search, this section can be reduced to only storingclassifier_paramsin a normalized form.
This issue also appears on line 143 of the same file.
if isinstance(classifier_params, dict):
self.classifier_params = dict(classifier_params)
self.clf = classifiers[classifier][0](**classifier_params)
elif classifier_params == "auto":
self.auto_classifier_param = True
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 30 out of 31 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
kalelinear/transformer/_mpca.py:366
MPCA.transform()will currently raise an AttributeError if called beforefit(), because it accesses fitted attributes (e.g.n_dims_,mean_). As a scikit-learn style transformer it should raise aNotFittedErrorviacheck_is_fitted()instead.
if vectorize is None:
vectorize = self.vectorize
# reshape X to shape (1, I_1, I_2, ..., I_N) if X in shape (I_1, I_2, ..., I_N), i.e. n_samples = 1
kalelinear/transformer/_mpca.py:405
MPCA.inverse_transform()also assumes the estimator has been fitted (usesoutput_shape_,idx_order_,proj_mats_,mean_) but doesn't check. Addingcheck_is_fitted()will produce a clearNotFittedErrorinstead of an AttributeError.
# reshape X to tensor in shape (n_samples, self.output_shape_) if X has been unfolded
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 30 out of 31 changed files in this pull request and generated no new comments.
Suppressed comments (5)
kalelinear/transformer/_mpca.py:158
- The MPCA docstring example is internally inconsistent:
MPCA()defaults tovectorize=False, sompca.transform(X)should return a tensor (same shape asfit_transform), not a 2D array. Also,mpca.transform(X, vectorize=True)will only return 50 features ifn_componentsis set, but the example never sets it.
>>> mpca = MPCA()
>>> X_projected = mpca.fit_transform(X)
>>> X_projected.shape
(40, 18, 23, 18)
>>> X_projected = mpca.transform(X)
kalelinear/utils/_base.py:15
- Renaming
lap_normparameters ton_neighbors/normalizebreaks callers that still pass the previous keyword names (n_neighbour/normalise). Sincelap_normis exported fromkalelinear.utils, keeping backward-compatible keyword aliases avoids a hard break for downstream users.
def lap_norm(X, n_neighbors=3, metric="cosine", mode="distance", normalize=True):
"""[summary]
Parameters
----------
kalelinear/estimator/base.py:24
BaseKaleEstimator.__init__was renamed tok_neighbors, but it still accepts**kwargs. If a downstream caller still passesk_neighbour=..., it will be silently swallowed by**kwargsand ignored (leavingself.k_neighborsat the default), which is very hard to debug. Consider mapping the legacy keyword ontok_neighbors(and ideally warning).
def __init__(
self,
kernel="linear",
k_neighbors=5,
manifold_metric="cosine",
kalelinear/pipeline/mpca_trainer.py:168
- For
classifier="svc"withclassifier_params="auto", the grid search is run withprobability=False(default), but after selection the code togglesprobability=Trueand refits. This means hyperparameter selection is performed under different training settings than the final model, and it also causes an extra (potentially expensive) SVC fit. Prefer settingprobability=Trueon the estimator used byGridSearchCVand skipping the redundant refit.
if self.auto_classifier_param:
param_grid = {name: list(values) for name, values in self.classifier_param_grid.items()}
extra_c = 1 / X.shape[0]
if extra_c not in param_grid["C"]:
param_grid["C"].append(extra_c)
kalelinear/pipeline/mpca_trainer.py:125
MPCATrainer.__init__has two separateauto/dictinitialization blocks for the classifier (one starting at line 99 and another starting at line 116). This duplication makes the init path harder to reason about and increases the risk of diverging behavior if only one block is updated later.
if self.classifier_param_grid is None:
self.classifier_param_grid = {
param_name: list(values) for param_name, values in classifiers[classifier][1].items()
}
self.grid_search = GridSearchCV(
MPCATrainerpipeline under a newpipelineAPI.MPCA