feat: vcov and precision matrices + move float ops to f64#86
Merged
Neclow merged 12 commits intosbhattlab:mainfrom Jul 2, 2025
Merged
feat: vcov and precision matrices + move float ops to f64#86Neclow merged 12 commits intosbhattlab:mainfrom
Neclow merged 12 commits intosbhattlab:mainfrom
Conversation
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR adds variance–covariance (vcv) and precision matrix support to both R and Python bindings, consolidates all floating-point operations in Rust to f64, and renames the “metrics” module to “stats” throughout the codebase.
- Introduce
vcv_from_vector,vcv_from_matrix, andprecisionfunctions (with Schur complement) in R and Rust, plus corresponding PythoncovandprecisionAPIs. - Convert all Rust matrix and graph code from
f32tof64to align with downstream R/Python and improve numerical precision. - Rename the R/Python “metrics” module to “stats”, update imports/tests, and remove obsolete
test_metrics.R.
Reviewed Changes
Copilot reviewed 32 out of 33 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| r-phylo2vec/tests/testthat/test_stats.R | Add R tests for vcv and precision functions |
| r-phylo2vec/src/rust/src/lib.rs | Switch f32 → f64, implement new vcv_from_* and pre_precision_from_* exports |
| r-phylo2vec/R/stats.R | Add R wrappers for vcv and precision (Schur complement) |
| py-phylo2vec/phylo2vec/stats/nodewise.py | Rename metrics → stats, implement cov and precision APIs |
| py-phylo2vec/src/lib.rs | Update Python bindings for f64 arrays and new functions |
Comments suppressed due to low confidence (2)
py-phylo2vec/phylo2vec/stats/nodewise.py:83
- The implementation references
core.vcvandcore.vcv_with_blsbutcoreis never imported; add an import such asimport phylo2vec._phylo2vec_core as core(orfrom phylo2vec import core) at the top of the file to avoid a NameError.
def cov(vector_or_matrix):
py-phylo2vec/phylo2vec/stats/nodewise.py:3
- [nitpick] The
warningsimport is unused in this module; consider removing it to keep the namespace clean.
import warnings
Collaborator
Author
|
R: renamed "cov" to "vcovp" to avoid collisions with base/stats (cov/vcov) and ape (vcv) |
Collaborator
Author
|
Note: the benchmarks are slightly worse now for matrix operations due to the f32 -> f64 conversion, although still within a reasonable margin in my opinion. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Features
Refactors
metricstostats:metricsnever made a lot of sense (cophenetic distances are not really a "metric" of a tree, so currently renamed tostatswhich will contain all sort of operations either between nodes (nodewisein the Python package) or between trees (treewise, upcoming...).Notes
scipy.linalg.solve(..., assume_a="pos")instead ofnp.linalg.solveinprecisionis 50% faster for n = 1000 leaves