Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #20 +/- ##
==========================================
+ Coverage 88.71% 90.19% +1.47%
==========================================
Files 21 24 +3
Lines 1506 1886 +380
==========================================
+ Hits 1336 1701 +365
- Misses 170 185 +15 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR adds two new multiblock feature transformers—CIFE and AJIVE—to the kalelinear.transformer API, along with tests and documentation updates so they can be used via both kalelinear.transformer and the PyKale-style kalelinear.embed module.
Changes:
- Implement CIFE and AJIVE, plus a shared multiblock base/validation layer.
- Add unit tests and shared synthetic multiblock dataset generator for validating common/individual subspace recovery.
- Update README, tutorials, and Sphinx API docs to surface the new transformers.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| TUTORIALS.md | Adds a usage example for CIFE/AJIVE common + individual feature extraction. |
| README.md | Lists CIFE/AJIVE as supported transformers and adds citations. |
| tests/utils/test_utils.py | Adds a synthetic multiblock dataset generator for common/individual structure. |
| tests/transformer/test_cife.py | Adds test coverage for CIFE fit/transform behavior and validation. |
| tests/transformer/test_ajive.py | Adds test coverage for AJIVE fit/transform behavior and validation. |
| tests/test_public_api.py | Ensures new transformers are exposed via the public API modules. |
| kalelinear/transformer/_multiblock.py | Introduces shared multiblock input handling and a base transformer class. |
| kalelinear/transformer/_cife.py | Implements the CIFE algorithm and its COBE-based common subspace extraction. |
| kalelinear/transformer/_ajive.py | Implements the AJIVE algorithm including Wedin-bound based rank selection. |
| kalelinear/transformer/init.py | Exposes CIFE and AJIVE in the transformer package namespace. |
| kalelinear/embed.py | Exposes CIFE and AJIVE via the PyKale-style embed module. |
| docs/source/introduction.rst | Updates the “Main Features” list to include CIFE/AJIVE. |
| docs/source/api_transformers.rst | Adds API doc entries for CIFE and AJIVE. |
| docs/source/api_embed.rst | Adds API doc entries for CIFE and AJIVE under kalelinear.embed. |
💡 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 14 out of 14 changed files in this pull request and generated no new comments.
Suppressed comments (3)
Previously missed (3) — in code that hasn't changed since the last review.
kalelinear/transformer/_multiblock.py:167
transform()uses_check_multiblock_input(X)for list/tuple inputs, which enforces a minimum of two blocks. This prevents projecting a single new block at transform-time even though the common projection does not require multiple blocks (and the docstring suggests any “list of blocks” is acceptable). Consider validating list inputs here without the “>= 2 blocks” constraint, and just stack the blocks for projection.
if isinstance(X, (list, tuple)):
blocks, _ = _check_multiblock_input(X)
X_stacked = np.vstack(blocks)
kalelinear/transformer/_ajive.py:208
percentileis a configurable parameter, but the branch choosing betweenrandom_ssv_boundand the Wedin-based bound compares against a hard-coded 5th percentile (np.percentile(wedin_ssv_bounds, 5)). This makes behavior inconsistent whenpercentileis not 5 and likely ignores the user-configured setting.
wedin_ssv_bound = np.percentile(wedin_ssv_bounds, self.percentile)
random_ssvs = _random_direction_ssv(D, ranks, 100, self.random_state_)
random_ssv_bound = np.percentile(random_ssvs, 95)
if random_ssv_bound > np.percentile(wedin_ssv_bounds, 5):
joint_rank = int(np.sum(s_stacked**2 + _FERROR > random_ssv_bound))
tests/utils/test_utils.py:133
make_common_individual_datasetis parameterized byn_blocks, but it indexesindividual_ranks[k]/n_samples[k]without validating their lengths. Calling it with a differentn_blocksthan the default will raise anIndexErrorinstead of a clear error message.
for k in range(n_blocks):
individual_basis, _ = np.linalg.qr(random_state.randn(n_features, individual_ranks[k]))
individual_basis -= common_basis @ (common_basis.T @ individual_basis)
individual_basis, _ = np.linalg.qr(individual_basis)
block = random_state.randn(n_samples[k], n_common) @ common_basis.T
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 23 changed files in this pull request and generated no new comments.
Suppressed comments (5)
Previously missed (2) — in code that hasn't changed since the last review.
TUTORIALS.md:120
- This example fails before producing
z_common: each random 50×10 block has full feature rank, andCIFEexplicitly raises whenpca_dimis unset for such blocks (see_column_space_basis). Please either construct rank-deficient blocks with planted common/individual subspaces or pass an appropriatepca_dim; the former would also make the tutorial demonstrate actual common structure.
X = np.vstack([rng.normal(size=(50, 10)) for _ in range(3)])
groups = np.repeat([0, 1, 2], 50)
cife = CIFE(random_state=0)
kalelinear/transformer/_ajive.py:163
- For a nonempty all-zero block, the denominator here is zero, producing NaNs;
searchsortedthen selects rank 1 and the downstream Wedin calculation divides by a zero singular value. This can ultimately report arbitrary full-rank individual components for zero data. Treat a zero-energy block as rank 0 so the existing positive-rank validation rejects it cleanly.
singular_values = np.linalg.svd(block, compute_uv=False)
if singular_values.size == 0:
ranks.append(0)
continue
explained = np.cumsum(singular_values**2) / np.sum(singular_values**2)
docs/source/api_embed.rst:1
- Deleting this page leaves
docs/source/api.rst:9pointing to the nonexistentapi_embeddocument, so Sphinx will report an unknown-document reference on the compatibility page retained for existing links. Remove or redirect that reference as part of this deletion.
kalelinear/transformer/_multiblock.py:197 - The block order is recomputed from first appearance on every call, but fit-time block IDs are not retained. If fitting sees groups in order
[2, 0, 1]and transformation sees[0, 1, 2], the laterzipsilently applies each block to the wrong block-specific basis. Persist the fit-time IDs and reorder/validate incoming stacked groups against them.
blocks, _ = _check_multiblock_input(X, groups)
kalelinear/transformer/_multiblock.py:89
- Infinite values pass this validation and are cast to an implementation-dependent integer. In CIFE,
[np.inf, ...]can consequently leave a large negative value inindividual_ranks_despite the documented non-negative component counts. Reject infinities before converting the ranks to integers.
if np.any(np.isnan(ranks)):
raise ValueError(f"{name} must not contain NaN values.")
There was a problem hiding this comment.
🔵 Needs a closer look
AJIVE’s perturbation calculations and CIFE’s zero-common-rank path contain correctness issues.
Review details
Suppressed comments (3)
Previously missed (3) — in code that hasn't changed since the last review.
kalelinear/transformer/_ajive.py:37
initial_ranksmay legally exceed the dimension of this orthogonal complement. In that case this loop requests more mutually orthogonal directions than exist; oncecurrentspans the ambient space, the retry can loop forever for exact arithmetic or normalize round-off noise. Limit the number of orthogonal directions to the null-space dimension (or avoid requiring the sampled null directions to be mutually orthogonal).
for _ in range(basis.shape[1]):
kalelinear/transformer/_ajive.py:49
- This defaults to the Frobenius norm for a matrix, but the Wedin perturbation bound requires the matrix spectral (2-)norm; the referenced AJIVE implementation's MATLAB
norm(data*nulldir)also uses that norm. With multiple sampled directions, the Frobenius value is systematically larger and changes joint-rank selection.
null_norms[i] = np.linalg.norm(data @ directions)
kalelinear/transformer/_cife.py:77
n_common_components=0is accepted by the estimator constraints, but the zero-component return is reached only after every block passes_column_space_basis. Consequently, full-rank blocks still raise thepca_dimerror even though no common subspace is requested and individual components can be computed directly. Handle the zero case before building the column-space bases.
for Y in blocks:
basis, rank = _column_space_basis(Y, pca_dim=pca_dim)
- Files reviewed: 22/23 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Description
Add two new algorithms, AJIVE and CIFE, under the transformer API
Status
Work in progress
Types of changes
docsupdated.