Skip to content

MPCA pipeline - #18

Open
shuo-zhou wants to merge 11 commits into
mainfrom
mpca-pipeline
Open

MPCA pipeline#18
shuo-zhou wants to merge 11 commits into
mainfrom
mpca-pipeline

Conversation

@shuo-zhou

@shuo-zhou shuo-zhou commented Aug 8, 2026

Copy link
Copy Markdown
Member
  1. Added MPCATrainer pipeline under a new pipeline API.
  2. Rrefactor MPCA
  3. Improve variable naming consistency.
  4. Update corresponding tests

@github-advanced-security

Copy link
Copy Markdown

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:

  • The 'Security' tab will display more code scanning analysis results (e.g., for the default branch).
  • Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results.
  • You will be able to see the analysis results for the pull request's branch on this overview once the scans have completed and the checks have passed.

For more information about GitHub Code Scanning, check out the documentation.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 MPCATrainer pipeline (MPCA → feature selection → classifier) for end-to-end training.
  • Refactor MPCA to 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.

Comment thread kalelinear/pipeline/mpca_trainer.py
Comment thread kalelinear/pipeline/mpca_trainer.py
Comment thread kalelinear/transformer/_mpca.py
Comment thread kalelinear/transformer/_mpca.py
@shuo-zhou shuo-zhou changed the title Mpca pipeline MPCA pipeline Aug 9, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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() mutates self.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 passing k_neighbour= will currently be silently ignored (captured in kwargs) rather than controlling k_neighbors. To avoid a hard-to-diagnose behavior change, consider translating k_neighbourk_neighbors with 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 passing k_neighbour= will be accepted but silently ignored (captured in kwargs), changing behavior without warning. Consider translating k_neighbourk_neighbors with 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 passing k_neighbour= will now be accepted but ignored (captured in kwargs). Translating k_neighbourk_neighbors with 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_params is None, self.mpca_params is set to the module-level default_mpca_params dict by reference. This shares a mutable dict across all MPCATrainer instances, 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 on self.
        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 parameter self.n_features (and uses self.feature_order without 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=None will still lead to slicing with None (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_neighbourn_neighbors and normalisenormalize is a breaking change for a public utility (lap_norm is exported from kalelinear.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 **kwargs in the signature, callers using the legacy keyword k_neighbour will not get a TypeError anymore; the value will be silently ignored and stored in self.kwargs, changing behavior without warning. Explicitly map k_neighbourk_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 x for the feature tensor while most other updated tests/docs use X (and Xs/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)

@codecov

codecov Bot commented Aug 12, 2026

Copy link
Copy Markdown

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 ☂️

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 by idx_order_. Re-applying self.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 applies feature_order_ again). Use an identity ordering when n_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_norm keyword arguments (n_neighbourn_neighbors, normalisenormalize) 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_params validation/initialization block duplicates the logic already handled just above (and reassigns self.clf / auto_classifier_param). It increases maintenance risk without adding behavior. Since the first block already validates and sets up clf/grid_search, this section can be reduced to only storing classifier_params in 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

Comment thread kalelinear/transformer/_mpca.py

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 before fit(), because it accesses fitted attributes (e.g. n_dims_, mean_). As a scikit-learn style transformer it should raise a NotFittedError via check_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 (uses output_shape_, idx_order_, proj_mats_, mean_) but doesn't check. Adding check_is_fitted() will produce a clear NotFittedError instead of an AttributeError.
        # reshape X to tensor in shape (n_samples, self.output_shape_) if X has been unfolded

Comment thread kalelinear/pipeline/mpca_trainer.py
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 to vectorize=False, so mpca.transform(X) should return a tensor (same shape as fit_transform), not a 2D array. Also, mpca.transform(X, vectorize=True) will only return 50 features if n_components is 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_norm parameters to n_neighbors / normalize breaks callers that still pass the previous keyword names (n_neighbour / normalise). Since lap_norm is exported from kalelinear.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 to k_neighbors, but it still accepts **kwargs. If a downstream caller still passes k_neighbour=..., it will be silently swallowed by **kwargs and ignored (leaving self.k_neighbors at the default), which is very hard to debug. Consider mapping the legacy keyword onto k_neighbors (and ideally warning).
    def __init__(
        self,
        kernel="linear",
        k_neighbors=5,
        manifold_metric="cosine",

kalelinear/pipeline/mpca_trainer.py:168

  • For classifier="svc" with classifier_params="auto", the grid search is run with probability=False (default), but after selection the code toggles probability=True and 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 setting probability=True on the estimator used by GridSearchCV and 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 separate auto/dict initialization 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(

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.

3 participants