Enhancement/crate level docs for `alignment`, `alphabets` and `asr` modules by junniest · Pull Request #185 · acg-team/phyne

junniest · 2026-09-16T14:15:51Z

Properly documented the alignment, alphabets and asr modules, including missing docstrings/tests and module-level docs for each.
Includes slight changes in the public API by making some string constants pub(crate) instead of pub.

alignment: hide module-level docs from re-imports to reduce clutter. Documentation of re-imported stuff will be inlined.

alignment: replace Vec<String> used to map node indices to sequence ids with a named type with a description

alignment: fix docstrings (use IDs instead of ids) for consistency among docs

alphabets: hide submodule docs for public re-export of the parsimony_set submodule

alphabets: add doctrings for static objects such as alphabets and pre-computed conditional probabilities

alphabets: replace incorrect use of HashSet for an empty parsimony set with the predefined ParsimonySet type

Added module-level docs for the ASR module with an example. Added an example tree file with ancestral node labels.

Copilot

🟡 Changes recommended

Resolve the public API regression and crate-wide documentation-lint CI failures.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Adds crate-level documentation and examples for alignment, alphabets, and asr, plus ASR test coverage and fixture data.

Changes:

Enables crate-wide missing_docs warnings.
Adds module/API documentation and examples.
Adds an ancestral reconstruction test and Newick fixture.

File summaries

File	Summary
`phylo/src/lib.rs`	Enables missing-documentation warnings.
`phylo/src/asr/tests.rs`	Adds ancestral reconstruction test coverage.
`phylo/src/asr/mod.rs`	Adds ASR module documentation and example.
`phylo/src/alphabets/mod.rs`	Documents alphabet APIs and statics.
`phylo/src/alignment/mod.rs`	Documents alignment APIs and examples.
`phylo/examples/data/tree_with_ancestral_ids.newick`	Provides test tree data.

Review details

Suppressed comments (3)

phylo/src/alignment/mod.rs:187

The new description's phrase by its node index is grammatically incorrect and redundant; the node index is the selector for the sequence being updated.

    /// Updates the `Mapping` of the ancestral sequence associated with the given internal node by
    /// its node index.

phylo/src/alphabets/mod.rs:364

The new documentation contains the typo beused; it should be two words.

    /// Static definition of an unknown alphabet, can beused as a fallback when the alphabet of a

phylo/src/alphabets/mod.rs:456

This doc comment is indented with three spaces inside the lazy_static! block, so the repository's rustfmt check will rewrite it to four spaces and fail the formatting job. Please align it with the surrounding declarations.

   /// Static parsimony sets for each amino acid character.

Files reviewed: 6/6 changed files
Comments generated: 4
Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

github-actions · 2026-09-16T14:21:36Z

Test Results

991 tests +8 990 ✅ +8 14m 58s ⏱️ -26s
2 suites ±0 1 💤 ±0
1 files ±0 0 ❌ ±0

Results for commit a4678c5. ± Comparison against base commit 752d213.

This pull request removes 15 and adds 23 tests. Note that renamed tests count towards both.

src/alignment/mod.rs - alignment::MASA ‑ from_aligned (line 408)
src/alignment/mod.rs - alignment::MASA ‑ from_aligned_with_ancestral_unchecked (line 514)
src/alignment/mod.rs - alignment::MSA ‑ alphabet (line 191)
src/alignment/mod.rs - alignment::MSA ‑ from_aligned_unchecked (line 282)
src/alignment/mod.rs - alignment::MSA ‑ len (line 219)
src/alignment/mod.rs - alignment::MSA ‑ seq_count (line 247)
src/alphabets/mod.rs - alphabets::Alphabet ‑ ambiguous (line 112)
src/alphabets/mod.rs - alphabets::Alphabet ‑ char_encoding (line 156)
src/alphabets/mod.rs - alphabets::Alphabet ‑ gap_set (line 203)
src/alphabets/mod.rs - alphabets::Alphabet ‑ index (line 139)
…

alphabets::tests ‑ alphabet_names
asr::tests ‑ test_reconstruct_ancestral_seqs
src/alignment/mod.rs - alignment (line 17)
src/alignment/mod.rs - alignment::MASA ‑ from_aligned (line 498)
src/alignment/mod.rs - alignment::MASA ‑ from_aligned_with_ancestral_unchecked (line 604)
src/alignment/mod.rs - alignment::MSA ‑ alphabet (line 276)
src/alignment/mod.rs - alignment::MSA ‑ from_aligned_unchecked (line 367)
src/alignment/mod.rs - alignment::MSA ‑ len (line 304)
src/alignment/mod.rs - alignment::MSA ‑ seq_count (line 332)
src/alphabets/mod.rs - alphabets (line 12)
…

♻️ This comment has been updated with latest results.

codecov · 2026-09-16T14:21:44Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.65%. Comparing base (527040d) to head (32bf5a6).
⚠️ Report is 38 commits behind head on develop.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #185      +/-   ##
===========================================
+ Coverage    96.32%   96.65%   +0.33%     
===========================================
  Files           32       46      +14     
  Lines         4355     5475    +1120     
===========================================
+ Hits          4195     5292    +1097     
- Misses         160      183      +23

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

MattesMrzik · 2026-09-18T16:13:09Z

+//! Ancestral sequence reconstruction module.
+//!
+//! Provides the `AncestralSequenceReconstruction` trait for reconstructing ancestral sequences
+//! given a leaf alignment or an existing ancestral alignment and a phylogenetic tree.


existing ancestral alignment

Perhaps say that the ancestral sequences will be ignored in that case. Since its only passed as Alignment so we dont have access to the ancestral sequences.

MattesMrzik · 2026-09-18T16:26:17Z

+//! The Sequences object is used to store the sequences in the alignment, and the lengths of
+//! the sequences in the alignment must be consistent.


... and the lengths of the sequences in the alignment must be consistent.

But right now the lens of the seqs in the Sequences obj that is a field of the alignment must not be of equal length, as their gaps are removed. Only when we call from_aligned, passing a Sequences, must they be of equal length.

MattesMrzik · 2026-09-18T16:31:44Z

+//! The sequence IDs must be unique within the collection.
+//! The sequences stored in the object can be aligned or unaligned.
+//!
+//! Contains the [`Alignment`] and [`AncestralAlignment`] traits for representing and manipulating


Perhaps mention that the alignment and how its implemented here depends on a tree, since we are using NodeIdx for some its fields (like SeqMaps). So the alignment should always be seen as belonging to some tree.

MattesMrzik · 2026-09-18T16:33:39Z

+/// Represents the mapping of sequences for nodes in the tree as a `hashbrown::HashMap`
+/// where the key is the node index (of type [`NodeIdx`]) and the value is the [`Mapping`] of
+/// sequences for that node.
 pub type SeqMaps = HashMap<NodeIdx, Mapping>;


... tree ...

Perhaps link the Tree struct here?

MattesMrzik · 2026-09-18T16:39:27Z

+    /// Returns the `Mapping` of the ancestral sequence associated with the given internal node.
    fn ancestral_map(&self, node_idx: &NodeIdx) -> &Mapping;


Suggested change

/// Returns the `Mapping` of the ancestral sequence associated with the given internal node.

fn ancestral_map(&self, node_idx: &NodeIdx) -> &Mapping;

/// Returns the `Mapping` of the ancestral sequence corresponding to the given internal node index.

fn ancestral_map(&self, node_idx: &NodeIdx) -> &Mapping;

To match the doc for the leaf_map()

MattesMrzik · 2026-09-18T16:43:57Z

+/// Represents a multiple sequence alignment (MSA) with associated leaf mappings and internal alignments.
+///
+/// This structure stores the sequences and their corresponding mappings for leaf nodes,
+/// as well as the internal alignments and a mapping from node indices in the tree to sequence IDs.


... internal alignments ...

Perhaps mention that these are created according to the tree that was passed on creation.
This also applies for the MASA doc.

MattesMrzik · 2026-09-18T16:47:51Z

+pub(crate) static AMB_AMINOACIDS: &[u8] = b"BJZX";
+
+/// All single-letter symbols for valid non-ambiguous nucleotides used in the alphabet.
+/// This is legacy sorting, i.e., conforming with tools like RaXML and PhyML, and is used also


MattesMrzik · 2026-09-18T16:51:04Z

+/// The character representing a gap in the alphabet used in MSAs.
 pub static GAP: u8 = b'-';


Suggested change

/// The character representing a gap in the alphabet used in MSAs.

pub static GAP: u8 = b'-';

/// The character in the alphabet representing a gap as used in MSAs.

or The character in the alphabets representing a gap.

or The character representing a gap in MSAs.

MattesMrzik · 2026-09-18T16:54:08Z

 pub struct Alphabet {
+    /// Human-readable name of the alphabet.
    name: &'static str,
+    /// Unambiguous symbols in the alphabet, in matrix order.


Perhaps link the QMatrix here?

MattesMrzik · 2026-09-19T08:52:22Z

+pub(crate) static AMB_NUCLEOTIDES: &[u8] = b"RYSWKMBDHVNZX";
+
+/// The character representing an ambiguous symbol in both the amino acid and nucleotide alphabets.
+pub(crate) static AMB_CHAR: u8 = b'X';


In the upcoming simulation #140 , but also in parsimony_presence_absence, and EdgeSeqsReestimator I use these X chars as mention in the docs, since I want to tell the user which char will be used for indicating any char. This will not be possible when having this only pub(crate) and not pub. Same holds for the GAP.

junniest added 20 commits September 14, 2026 15:34

Add linter warning for missing documentation

91b87be

alignment: add submodule-level docs

166acd6

alignment: hide module-level docs from re-imports

10705c3

alignment: hide module-level docs from re-imports to reduce clutter. Documentation of re-imported stuff will be inlined.

alignment: add docstrings for structs and traits

5ac057a

alignment: add missing method docstrings

084f03e

alignment: replace Vec<String> with a named type

17ed17d

alignment: replace Vec<String> used to map node indices to sequence ids with a named type with a description

alignment: fix docstrings (IDs) for consistency

e8b55d4

alignment: fix docstrings (use IDs instead of ids) for consistency among docs

alphabets: hide submodule docs

502db1b

alphabets: hide submodule docs for public re-export of the parsimony_set submodule

alphabets: add doctrings for static objects

a0db349

alphabets: add doctrings for static objects such as alphabets and pre-computed conditional probabilities

alphabets: replace HashSet with ParsimonySet

202b842

alphabets: replace incorrect use of HashSet for an empty parsimony set with the predefined ParsimonySet type

alphabets: add method to return readable name

1415e36

alphabets: add doctest to name() method

89d5fd3

alphabets: add example for Alphabet::protein()

3f36b07

alphabets: add example for Alphabets::dna()

8c9ee40

alphabets: fix doctest for Alphabet::name()

a28160e

alphabets: add docs for private functions

99680fa

alignment: fix module-level doctest

0139b06

alphabet: add module-level docstring

1f4554f

asr: add module-level docs + example

12ae6c8

Added module-level docs for the ASR module with an example. Added an example tree file with ancestral node labels.

asr: add test identical to module-level doctest

d031819

junniest requested review from MattesMrzik and a lite review from Copilot September 16, 2026 14:15

Copilot started reviewing on behalf of junniest September 16, 2026 14:16

View session

Copilot AI reviewed Sep 16, 2026

View reviewed changes

Comment thread phylo/src/alphabets/mod.rs Resolved

Comment thread phylo/src/lib.rs Resolved

Comment thread phylo/src/alignment/mod.rs Resolved

Comment thread phylo/src/alphabets/mod.rs Resolved

junniest added 3 commits September 16, 2026 16:47

alignment: fix missing space in docstring

9fc04b6

alphabets: fix typos in docstrings

05d2497

alphabets: add test for Alphabet::name()

32bf5a6

MattesMrzik approved these changes Sep 18, 2026

View reviewed changes

MattesMrzik requested changes Sep 19, 2026

View reviewed changes

alignment: inline docs for Sequences and Aligner

a4678c5

		//! The Sequences object is used to store the sequences in the alignment, and the lengths of
		//! the sequences in the alignment must be consistent.

		/// Returns the `Mapping` of the ancestral sequence associated with the given internal node.
		fn ancestral_map(&self, node_idx: &NodeIdx) -> &Mapping;

		/// The character representing a gap in the alphabet used in MSAs.
		pub static GAP: u8 = b'-';

	/// The character representing a gap in the alphabet used in MSAs.
	pub static GAP: u8 = b'-';
	/// The character in the alphabet representing a gap as used in MSAs.

Conversation

junniest commented Sep 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (3)

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Sep 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results

Uh oh!

codecov Bot commented Sep 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

junniest commented Sep 16, 2026 •

edited

Loading

github-actions Bot commented Sep 16, 2026 •

edited

Loading

codecov Bot commented Sep 16, 2026 •

edited

Loading