feat: add extra core rust operations and remove python duplicates#15
Merged
Neclow merged 5 commits intosbhattlab:mainfrom May 1, 2025
Merged
feat: add extra core rust operations and remove python duplicates#15Neclow merged 5 commits intosbhattlab:mainfrom
Neclow merged 5 commits intosbhattlab:mainfrom
Conversation
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR introduces new core Rust operations for Newick string manipulation and ancestry conversion while cleaning up duplicate Python utility functions.
- Added new functions (from_ancestry, check_m, find_num_leaves, has_branch_lengths, has_parents, remove_branch_lengths, remove_parent_labels) to the Rust core and exposed them to Python.
- Removed duplicate implementations in the Python utilities and updated function registration in the module binding.
Reviewed Changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| py-phylo2vec/src/lib.rs | Added new Python bindings for core functions and re-ordered module registration. |
| py-phylo2vec/phylo2vec/utils/vector.py | Removed duplicate implementations for remove_leaf and add_leaf. |
| py-phylo2vec/phylo2vec/utils/newick.py | Replaced inline Newick functions with calls to core functions; potential issue in remove_parent_labels. |
| phylo2vec/src/tree_vec/ops/vector.rs | Added from_ancestry function for converting ancestry back to vector. |
| phylo2vec/src/tree_vec/ops/newick/mod.rs | Introduced functions for handling branch lengths and added tests for has_branch_lengths. |
| phylo2vec/src/tree_vec/ops/mod.rs | Updated exports to include from_ancestry and added ancestry conversion tests. |
Files not reviewed (1)
- docs/api.rst: Language not supported
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR introduces new core Rust operations for Newick string processing and vector conversions while removing duplicate Python utility functions.
- Adds new functions: from_ancestry, has_branch_lengths, has_parents, remove_branch_lengths, remove_parent_labels, check_m, find_num_leaves.
- Reorders and consolidates Python API bindings in the _phylo2vec_core module and removes duplicate implementations in the Python utils code.
Reviewed Changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| py-phylo2vec/src/lib.rs | Added new Python bindings and reorganized function registration order |
| py-phylo2vec/phylo2vec/utils/vector.py | Removed duplicate definitions and now alias functions from core |
| py-phylo2vec/phylo2vec/utils/newick.py | Eliminated legacy regex-based functions in favor of core implementations |
| phylo2vec/src/tree_vec/ops/vector.rs | Added the from_ancestry function that reorders cherries before building vector |
| phylo2vec/src/tree_vec/ops/newick/mod.rs | Introduced remove_branch_lengths and has_branch_lengths with tests |
| phylo2vec/src/tree_vec/ops/mod.rs | Updated re-exports to include the new from_ancestry function |
Files not reviewed (1)
- docs/api.rst: Language not supported
Comments suppressed due to low confidence (1)
phylo2vec/src/tree_vec/ops/newick/mod.rs:335
- [nitpick] Ensure that the documentation examples for remove_branch_lengths reflect the latest implementation (removing annotations via core functions) and consider adding edge cases in tests.
pub fn remove_branch_lengths(newick: &str) -> String {
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR introduces additional core Rust operations for handling Newick strings and vector/ancestry conversions, while simultaneously cleaning up duplicate Python utility functions.
- Added new functions: from_ancestry, has_branch_lengths, remove_branch_lengths, find_num_leaves, has_parents, remove_parent_labels, and check_m.
- Re-ordered and consolidated Python bindings in the Rust module to remove legacy/duplicate function exposures.
- Removed deprecated utility functions in the Python vector module.
Reviewed Changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| py-phylo2vec/src/lib.rs | Exposes new functions and reorders Python bindings to remove duplicates |
| py-phylo2vec/phylo2vec/utils/vector.py | Refactors duplicate leaf functions and renames core import for clarity |
| py-phylo2vec/phylo2vec/utils/newick.py | Delegates Newick manipulation functions to core implementations |
| py-phylo2vec/phylo2vec/utils/init.py | Removes obsolete utils and updates exposed functions |
| phylo2vec/src/tree_vec/ops/vector.rs | Introduces from_ancestry implementation with proper ordering of cherries |
| phylo2vec/src/tree_vec/ops/newick/mod.rs | Adds branch length handling functions and associated tests |
| phylo2vec/src/tree_vec/ops/mod.rs | Updates tests to incorporate the new from_ancestry functionality |
Files not reviewed (1)
- docs/api.rst: Language not supported
Comments suppressed due to low confidence (1)
phylo2vec/src/tree_vec/ops/vector.rs:138
- [nitpick] The temporary variable 'ancestry_clone' later used for ordering could be renamed to 'ordered_ancestry' to more clearly convey its purpose.
pub fn from_ancestry(ancestry: &Ancestry) -> Vec<usize> {
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.
Addition of 3 new functions:
has_branch_lengths: checks if a Newick string has branch lengthsremove_branch_lengths: removes branch length annotations from a Newick stringfrom_ancestryconverts an ancestry back to a vector (opposite ofget_ancestry)We also expose
check_mandremove_parent_labelsto the Python package, and remove duplicate utils/ functions.