refactor: unify newick --> matrix conversion in rust and tests in python#32
Merged
Neclow merged 1 commit intosbhattlab:mainfrom May 14, 2025
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR unifies the conversion of Newick strings to matrices in Rust for both cases (with and without parent nodes) and updates the corresponding Python tests accordingly. Key changes include:
- Merging the to_matrix and to_matrix_no_parents functionality into a single function.
- Adding tests in Python to verify correct behavior in both scenarios.
- Bumping the version in Cargo.toml and removing a now-unused dev dependency.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| py-phylo2vec/tests/test_base.py | Added new test (test_m2newick2m) to validate conversion logic. |
| phylo2vec/src/tree_vec/ops/matrix/mod.rs | Refactored to merge parent and no parent cases into one function. |
| phylo2vec/Cargo.toml | Version bump and removal of ndarray from dev-dependencies. |
Comments suppressed due to low confidence (1)
phylo2vec/Cargo.toml:17
- [nitpick] Since the ndarray dev dependency has been removed, consider updating any accompanying documentation or comments to reflect this change and ensure consistency.
ndarray = "*"
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.
Current Python API fails when calling from_newick when a Newick string has branch lengths but no parents. This commit unifies to_matrix/to_matrix_no_parents in Rust under one function (similar to to_vector) and adds relevant matrix tests in Python.