Conversation
Migrate small-parsimony scoring and the FitchCount transition-count algorithm from Cassiopeia into first-class pycea.tl functions operating on TreeData. Reuse pycea's Fitch-Hartigan machinery by extracting a shared _fitch_hartigan_downpass helper in ancestral_states.py, used by both the existing reconstruction and fitch_count's state-set computation. - parsimony: reconstructs states (via ancestral_states) or uses existing ones; returns an int for a single tree, a Series across trees. - fitch_count: returns an MxM count matrix, summed across trees. Wire both into the API, docs, changelog, and add the Quinn 2021 citation. Includes tests in tests/test_parsimony.py. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add module-level filterwarnings markers for warnings the tests intentionally trigger: the "no artists with labels" legend warning in the hex-color tree plots, and the small-category / multi-threaded-fork warnings in the ancestral_linkage plot tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Reduce test_ancestral_linkage from 71 test functions to 22, grouping redundant per-assertion tests into coherent per-feature tests (pairwise values, output structure, min_size, symmetrize, single-target, by_tree, permutation, alternative, permutation_mode, normalize) and parametrizing the brute-force regression checks over the ultrametric and non-ultrametric fixtures. Also add filterwarnings markers for the expected small-category and multi-threaded-fork warnings. Coverage of distinct behaviors is preserved. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- get_keyed_obs_data: pass through already-categorical array data unchanged instead of recoercing. - ancestral_linkage: accept Mapping trees, map obs via plain dicts, and annotate tree_distance/compute_scores calls for the type checker. - partition_test: support callable metrics, use np.atleast_2d for stat reshaping, and quiet ttest_ind typing. - pl.ancestral_linkage: clearer error when the stats key or figure is missing. - docs: register tl.ancestral_linkage in the API reference. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #59 +/- ##
==========================================
+ Coverage 93.74% 93.83% +0.09%
==========================================
Files 36 37 +1
Lines 3195 3344 +149
==========================================
+ Hits 2995 3138 +143
- Misses 200 206 +6
🚀 New features to boost your workflow:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: de6ef97837
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # Set leaf states then compute Fitch-Hartigan state sets | ||
| leaf_states = {node: values[node] for node in g.nodes if g.out_degree(node) == 0 and node in values.index} | ||
| nx.set_node_attributes(g, leaf_states, key) | ||
| _fitch_hartigan_downpass(g, key, missing_state, set_key="_fitch_set") |
There was a problem hiding this comment.
When a child clade has an ambiguous downpass set, a globally equally parsimonious solution can assign that child the parent's state even if that state is not in the child's downpass set. For example, a root with one child containing leaves 0,0 and another containing 1,2 has an equally parsimonious assignment root=right_child=0, but _fitch_set for the right child is only {1,2}, so the DP never counts those solutions and undercounts transitions for such multi-state trees.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is expected behavior, not a regression. fitch_count intentionally counts transitions over the Fitch–Hartigan solution set (states enumerable from the per-node Fitch sets), which is the space defined by the original FitchCount algorithm (Quinn et al. 2021). The _N_fitch_count/_C_fitch_count DP is ported unchanged from Cassiopeia and produces identical counts. Assignments like root=right_child=0 in your example are maximally parsimonious but lie outside the Fitch enumeration, so neither this implementation nor the reference counts them. Broadening to the full MP space would be a different algorithm (e.g. a Sankoff-based count) rather than FitchCount, so I've left the semantics faithful to the reference.
| g = nx.DiGraph(g.subgraph(nx.descendants(g, root) | {root}).copy()) | ||
| actual_root = root if root is not None else get_root(g) | ||
| # Set leaf states then compute Fitch-Hartigan state sets | ||
| leaf_states = {node: values[node] for node in g.nodes if g.out_degree(node) == 0 and node in values.index} |
There was a problem hiding this comment.
When key contains ordinary pandas missing values and missing_state is left as the default, this still writes NaN into leaf states; observed was computed with dropna(), so NaN is absent from label_to_j, and _N_fitch_count later raises KeyError: nan instead of treating the missing leaf as the documented wildcard. This affects any categorical column with standard missing observations unless users pre-encode a custom sentinel.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f6e90c0: fitch_count now coerces pandas NaN leaf values to the missing sentinel so they're treated as Fitch wildcards instead of raising KeyError. Added a regression test.
| elif callable(metric): | ||
| metric_fn = metric |
There was a problem hiding this comment.
For a plain callable metric (which the overload accepts as _MetricFn), assigning the function directly means the later unconditional calls to metric_fn.pairwise(...) fail with AttributeError before a result is produced, even when test=None. Either wrap custom functions in a pairwise adapter or branch at the call sites so callable metrics are actually invoked.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f6e90c0: callable metrics are now wrapped in a _CallableMetric adapter that provides .pairwise, so a plain metric(a, b) callable works instead of raising AttributeError. Added a test asserting it matches the built-in mean_difference.
Set typehints_document_overloads = False so sphinx-autodoc-typehints no longer emits every @typing.overload signature. This restores the short description in the API summary table for overloaded functions (parsimony, fitch_count, autocorr, clades, distance, ...) and shows a single implementation signature on each function's page instead of the copy-only overload stubs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…partition_test - fitch_count: coerce pandas NaN leaf values to the missing sentinel so missing observations are treated as Fitch wildcards instead of raising KeyError when missing_state is left at the default. - partition_test: wrap a user-supplied callable metric in a _CallableMetric adapter that provides the .pairwise API, so callable metrics no longer fail with AttributeError. - Add tests covering both paths. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 participant
Summary
Migrates small-parsimony scoring and the FitchCount algorithm from Cassiopeia into pycea, cleans up the test suite, and cuts the v0.3.0 release.
New functions (
pycea.tl)parsimony— small-parsimony score of a categorical character. Reconstructs ancestral states viaancestral_states(defaultfitch_hartigan) or uses states already on the tree (reconstruct=False). Returns anintfor a single tree, aSeriesacross trees; also stored intdata.uns.fitch_count— FitchCount transition-count algorithm across all equally-parsimonious solutions, returning an MxMDataFramesummed across trees.Both reuse pycea's Fitch–Hartigan machinery: the down-pass was extracted into a shared
_fitch_hartigan_downpasshelper inancestral_states.py(behavior-preserving for existing reconstruction). Wired into the API, docs, changelog, and the Quinn 2021 citation added.Test suite cleanup
filterwarningsmarkers (small-category linkage, multi-threaded-fork, empty-legend). Suite-wide warning count is now 0.test_ancestral_linkage.pyfrom 71 test functions to 22, grouping per-assertion tests into coherent per-feature tests and parametrizing the brute-force regressions over the ultrametric/non-ultrametric fixtures. Distinct behavior coverage preserved.Other
tl/pl(get_keyed_obs_data,ancestral_linkage,partition_test,pl.ancestral_linkage).pyproject.tomlversion bump).Test plan
conda run -n pycea python -m pytest tests/ --ignore-glob='**/._*.py'→ passing, 0 warnings.tests/test_parsimony.pycovers both functions, including hand-computed FitchCount matrices and the multi-tree summation.🤖 Generated with Claude Code