Add PaCMAP dimensionality reduction (closes #173) - #193
Conversation
Add a `calculate_pacmap` method to `EmbeddingBase` and a "pacmap" option to `dimension_plotter`, mirroring the existing PCA/t-SNE/UMAP reducers. Adds `pacmap` as a dependency and unit tests for the new reducer. Closes WMD-group#173
|
Warning Review limit reached
Next review available in: 33 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughAdds PaCMAP as a fourth dimensionality reduction option. The ChangesPaCMAP Dimensionality Reduction
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds PaCMAP dimensionality reduction support to the embeddings API and plotting utilities, along with tests and dependency updates.
Changes:
- Introduce
calculate_pacmap()on embeddings (PaCMAP projection). - Add
"pacmap"as an option todimension_plotter(). - Add unit tests for PaCMAP in core and plotter tests; add
pacmapdependency.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/elementembeddings/_base.py | Adds calculate_pacmap() implementation backed by pacmap.PaCMAP. |
| src/elementembeddings/plotter.py | Extends dimension_plotter() to accept reducer="pacmap". |
| src/elementembeddings/tests/test_core.py | Adds deterministic(ish) PaCMAP tests for shape and reproducibility. |
| src/elementembeddings/tests/test_plotter.py | Adds plotter test case for the PaCMAP reducer option. |
| pyproject.toml | Adds pacmap to required dependencies. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def test_PaCMAP(self): | ||
| """Test the PaCMAP function.""" | ||
| pacmap_params = {"random_state": 42} | ||
| assert isinstance(self.test_matscholar.calculate_pacmap(), np.ndarray) | ||
| assert self.test_matscholar.calculate_pacmap().shape == ( | ||
| len(self.test_matscholar.element_list), | ||
| 2, | ||
| ) | ||
| pacmap1 = self.test_matscholar.calculate_pacmap(**pacmap_params) | ||
| pacmap2 = self.test_matscholar.calculate_pacmap(**pacmap_params) | ||
| assert (pacmap1 == pacmap2).all() |
There was a problem hiding this comment.
Kept exact equality to match the existing test_PCA / test_tSNE / test_UMAP determinism checks, which all assert (x == y).all(). I verified PaCMAP is bit-reproducible within a process under a fixed random_state (same kNN backend, two calls → identical arrays), so the exact check is stable here. Happy to switch all four reducer tests to np.testing.assert_allclose together if you'd prefer a tolerance-based convention.
| warnings.warn( | ||
| """It is recommended to scale the embeddings | ||
| before projecting with PaCMAP. | ||
| To do so, set `standardise=True`.""", | ||
| ) |
There was a problem hiding this comment.
The warning mirrors the wording and triple-quoted format of the existing calculate_pca / calculate_tsne / calculate_umap warnings verbatim, so I kept it identical for consistency. On stacklevel: B028 is in the ignore list in pyproject.toml and none of the sibling warnings set it, so adding it only here would be inconsistent. Happy to do a repo-wide stacklevel=2 pass (and drop the B028 ignore) as a separate cleanup if you'd prefer.
| def calculate_pacmap( | ||
| self, | ||
| n_components: int = 2, | ||
| standardise: bool = True, | ||
| init: str = "pca", | ||
| **kwargs, | ||
| ): |
There was a problem hiding this comment.
Left off for consistency with the sibling reducers — calculate_pca / calculate_tsne / calculate_umap are also unannotated. Glad to add -> np.ndarray across all four reducer methods if you'd like them annotated together.
| reducer (str): The dimensionality reduction algorithm to use. One of | ||
| "umap", "tsne", "pca" or "pacmap", by default "umap" |
There was a problem hiding this comment.
Done in 6089661 — the dispatch ValueError now includes the invalid value and lists the accepted reducers ('umap', 'tsne', 'pca', 'pacmap'). On the docstring: the accepted-values continuation line is indented to match the existing convention in this function's docstring (e.g. the reducer_params description wraps at the same level), so I kept it consistent with the surrounding args.
| msg = "Unrecognised reducer." | ||
| raise ValueError(msg) |
There was a problem hiding this comment.
Addressed in 6089661 — the dispatch ValueError now names the invalid value and the accepted reducers (including pacmap).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/elementembeddings/_base.py`:
- Around line 285-289: The warnings.warn() call for the PaCMAP scaling
recommendation is missing the stacklevel parameter, which causes the warning to
point to the library internals instead of the caller's code location. Add the
stacklevel parameter to the warnings.warn() call that contains the message about
standardising embeddings before projecting with PaCMAP. Set stacklevel to an
appropriate value (typically 2) so that the warning appears to originate from
the caller's code rather than from within the library itself.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0616cd87-f5ea-46aa-b168-85c31e9dbba1
📒 Files selected for processing (5)
pyproject.tomlsrc/elementembeddings/_base.pysrc/elementembeddings/plotter.pysrc/elementembeddings/tests/test_core.pysrc/elementembeddings/tests/test_plotter.py
List the accepted reducer names (including pacmap) and include the invalid value in the ValueError, per PR review feedback.
Resolves the rename of src/elementembeddings/tests/ to tests/: the new PaCMAP tests are carried over into tests/test_core.py and tests/test_plotter.py.
|
Thanks for this @ali-elite. Will get it merged now |
Summary
Adds PaCMAP (Pairwise Controlled Manifold Approximation and Projection) as a dimensionality-reduction option, alongside the existing PCA / t-SNE / UMAP reducers. Closes #173.
Changes
EmbeddingBase.calculate_pacmap(n_components=2, standardise=True, init="pca", **kwargs)in_base.py, following the same standardise-then-project pattern ascalculate_pca/calculate_tsne/calculate_umap.**kwargsare forwarded to thePaCMAPconstructor;initis forwarded toPaCMAP.fit_transform(PaCMAP exposes initialisation there rather than on the constructor), defaulting to"pca".dimension_plotter(..., reducer="pacmap")support inplotter.py, with the docstring updated to list the available reducers.pacmap>=0.7.0added to project dependencies inpyproject.toml.test_PaCMAPintest_core.py(return type,(n_elements, 2)shape, and determinism under a fixedrandom_state), and areducer="pacmap"assertion added to thedimension_plotterdispatch test intest_plotter.py.Testing
ruff(v0.15.2)check+format: clean.codespell: clean.I followed the convention of not committing a baseline image for the plotter test (matching the commented-out UMAP image test), instead asserting the dispatch returns a
plt.Axes. Happy to adjust the API surface (e.g. exposinginitdifferently) or add a tutorial-notebook example if you'd prefer.Summary by CodeRabbit
New Features
Tests