Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the core Rust components of the library by reorganizing module structures, splitting functionality into dedicated modules (e.g. vector, matrix, newick, tree_vec), and updating conversions and validations. Key changes include:
- Moving and renaming utility functions from the old “utils” module into more targeted modules (e.g. vector::base, matrix::base, newick).
- Updating conversion functions between matrix and Newick representations with improved use of ndarray views and error handling.
- Adjusting benchmarks and Python demo assets to work with the revised module structure.
Reviewed Changes
Copilot reviewed 28 out of 30 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/lib.rs | Updates module re-exports and refactors dependency on the old utils. |
| src/newick/mod.rs | Renames and refactors Newick parsing and formatting functions. |
| src/matrix/convert.rs | Adds conversion functions from Newick to matrix and vice versa. |
| src/vector/convert.rs | Refactors vector conversion functions (e.g. to_newick, from_newick). |
| src/tree_vec/mod.rs | Updates TreeVec methods to delegate to the new convert functions. |
| benches/benchmarks/core.rs | Adjusts benchmarks to use the refactored functions. |
| Cargo.toml | Updates dependency on ndarray and other revisions. |
| docs/demo.ipynb | Updates demo cells to reflect new function signatures and type casts. |
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.
I propose a refactoring of the codebase with a stronger separation of concerns based what each functions does:
As
tree_vecis not used, it makes sense (for me, at least) to isolate it somewhere else instead of making it the "super-class". The current structure also mimicks a bit more the current python API, so it should be easier to parse.Features
Includes #81, which ports matrix operations to ndarray/numpy. See the issue for more details.
Side effects:
Diagram
Previous architecture:
New proposed architecture:
Closed issues
closes #81 and closes #80