Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #20 +/- ##
==========================================
+ Coverage 88.71% 90.39% +1.68%
==========================================
Files 21 24 +3
Lines 1506 1895 +389
==========================================
+ Hits 1336 1713 +377
- Misses 170 182 +12 ☔ 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
There was a problem hiding this comment.
🟡 Changes recommended
AJIVE joint-rank selection currently violates the cited algorithm’s rank bound and threshold rule.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
kalelinear/transformer/_ajive.py:220
- Taking
max(wedin_ssv_bound, random_ssv_bound)does not match AJIVE's cited rank-selection rule. The reference uses the random bound only when it exceeds the 5th percentile Wedin bound; otherwise it uses the requested Wedin percentile. Withpercentile > 5, this implementation can choose a larger threshold and under-estimate the joint rank whenever the random bound exceeds Wedin's 5th percentile but not the requested percentile.
# Take the more conservative (larger) of the two perturbation bounds,
# following the reference implementation: max(wedin, random).
joint_threshold = max(wedin_ssv_bound, random_ssv_bound)
- Files reviewed: 22/23 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
🔵 Needs a closer look
Explicit AJIVE ranks allow zero-energy blocks to produce NaN bounds and invalid individual components.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
kalelinear/transformer/_ajive.py:193
- An explicit
initial_ranksvalue bypasses the zero-energy check in_resolve_initial_ranks. For an all-zero block,_wedin_angle_bounddivides0 / S[-1](S[-1]is zero), producing NaN bounds; the zero reconstruction threshold then causes_FERRORto classify every zero singular value as individual signal. Reject zero-energy blocks here regardless of how the ranks were supplied.
ranks = self._resolve_initial_ranks(blocks)
if np.any(ranks < 1):
raise ValueError("`initial_ranks` must contain positive values for every block.")
- Files reviewed: 22/23 changed files
- Comments generated: 0 new
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
AJIVE can produce NaN rank thresholds for rank-deficient blocks and performs unnecessary resampling for manually specified common ranks.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
kalelinear/transformer/_ajive.py:215
- When
n_common_componentsis supplied, the rank-selection result is overwritten below, but every block still performsn_resamplesWedin simulations and the additional random-direction simulation. With the default 1,000 resamples this dead work can dominate fitting; bypass generation of the perturbation bounds and random threshold entirely for a manually specified common rank.
angle_bounds.append(_wedin_angle_bound(block, self.n_resamples, U0, S0, V0, self.random_state_))
- Files reviewed: 22/23 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
AJIVE’s absolute rank-comparison epsilon makes common and individual rank selection depend on input scale.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
kalelinear/transformer/_ajive.py:274
- The fixed
_FERRORalso makes automatic individual-rank selection scale-dependent: after sufficiently downscaling otherwise identical blocks,s_individual + 1e-10exceeds each scaled threshold and can classify numerical zero directions as signal. Compare with a threshold-relative floating-point tolerance so changing measurement units does not change the selected ranks.
if ranks_spec is None:
rank = int(np.sum(s_individual + _FERROR > thresholds[n]))
- Files reviewed: 22/23 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
🔵 Needs a closer look
CIFE currently accepts tolerance and residual thresholds that can silently force invalid convergence and component-selection behavior.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
kalelinear/transformer/_cife.py:218
- Values above the natural unit interval are currently accepted even though they invalidate both decisions:
tol > 1makesabs(previous @ direction) > 1 - toltrue on the first iteration, whileepsilon > 1makes every residual direction count as common. Restrict these parameters so invalid settings fail validation instead of silently producing an unconverged or all-common decomposition.
- Files reviewed: 22/23 changed files
- Comments generated: 0 new
- Review effort level: Balanced
There was a problem hiding this comment.
🔵 Needs a closer look
Multiblock input dispatch rejects valid nested array-like matrices, and unsupported fit keywords are silently ignored.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
kalelinear/transformer/_multiblock.py:43
- The overload treats every Python list/tuple as a sequence of blocks, so a documented 2D array-like such as
X=[[1, 2], [3, 4], ...]is interpreted as one 1D block per row (or rejected immediately whengroupsis supplied). Distinguish a block sequence by checking that its elements are 2D; otherwise let the value follow the stacked-matrix path.
kalelinear/transformer/_multiblock.py:154 fit_paramsis never read or forwarded, so unsupported or misspelled keywords (for example,group=instead ofgroups=) are silently ignored while fitting a different model than requested. Remove the catch-all parameter so Python reports these mistakes, or explicitly validate/forward supported metadata.
- 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.