fix: several small bugs found in a static-analysis sweep#234
Merged
mmore500 merged 13 commits intoJul 13, 2026
Conversation
Node._set_edge() removed the node from its parent's _child_nodes list as a side effect of swapping in a new Edge object, without re-adding it anywhere or clearing _parent_node. Since a node's tail_node is derived from head_node._parent_node (not stored independently), this detachment had no bookkeeping purpose and only corrupted the tree: after 'node.edge = new_edge', the node vanished from its parent's children while node.parent_node still pointed at the old parent. Signed-off-by: Mike German <mike@stepsventures.com>
TreeSummarizer.summarize_node_ages_on_tree() checked 'if is_bipartitions_updated:' before re-encoding bipartitions, the opposite of every sibling summarization method in this module (which all skip the (deprecated) re-encode when the caller has already updated bipartitions). Passing is_bipartitions_updated=True therefore triggered an unwanted call to the deprecated Tree.encode_splits() instead of skipping it. Signed-off-by: Mike German <mike@stepsventures.com>
…_on_tree The 'force parent at least as old as its oldest child' step ran inside the same preorder pass used to assign ages, so it compared a node's age against children that the preorder traversal had not visited yet (still None), raising a TypeError on essentially any tree. Move the adjustment to its own postorder pass, run after all ages are assigned. Signed-off-by: Mike German <mike@stepsventures.com>
_calculate_standardized_effect_size() passed is_shuffle_phylogenetic_distances instead of is_shuffle_phylogenetic_path_steps to shuffle_taxa(). For is_weighted_edge_distances=False, this meant both shuffle flags were False, so the null-model replicates were never randomized at all, silently producing a null_model_sd of 0 for standardized effect size calculations on unweighted (edge-count) distances. Signed-off-by: Mike German <mike@stepsventures.com>
simulate_discrete_chars() accepts a 'root_states' parameter but passed 'root_states=None' on to DiscreteCharacterEvolver.evolve_states(), silently discarding it and always simulating a random root sequence from the model's stationary distribution instead of the caller's requested states. Signed-off-by: Mike German <mike@stepsventures.com>
…ndex
The unequal-length-sequences error used a '%d' printf-style
placeholder but called '.format()' on the string (which only
substitutes '{}' placeholders), so the raised ValueError always showed
the literal, unhelpful '%d' instead of the offending matrix's index.
Signed-off-by: Mike German <mike@stepsventures.com>
The character-subset label de-duplication loop computed a new candidate into 'label' but checked/looped on 'cs_label', which was never reassigned. Any label collision (e.g. concatenating two matrices with the same or default label) caused an infinite loop. Signed-off-by: Mike German <mike@stepsventures.com>
append_taxon_sequence() referenced 'self.default_symbol_state_map', an attribute that does not exist anywhere on CharacterMatrix or its subclasses, so any call with a non-empty 'state_symbols' argument raised an AttributeError. Use the existing 'default_state_alphabet.full_symbol_state_map' lookup instead. Signed-off-by: Mike German <mike@stepsventures.com>
__setitem__() and __delitem__() called dict.__setitem__()/ dict.__delitem__() directly, bypassing OrderedDict's internal bookkeeping. Reads (__getitem__, __contains__, get) were unaffected, but iteration (items()/keys()/values()/__iter__(), and thus len() staying correct while iteration went empty) was silently broken after any mutation. Signed-off-by: Mike German <mike@stepsventures.com>
Taxon.__init__() unconditionally set 'self.comments = []' after the if/else branch, even for the Taxon-from-Taxon copy path, which had just deep-copied 'comments' (along with the rest of __dict__) from the source Taxon a few lines earlier. The unconditional reset silently discarded it, so Taxon(other_taxon), copy.deepcopy(), and .clone() all dropped comments on the copy. Signed-off-by: Mike German <mike@stepsventures.com>
…length When a node's own edge length is None but it has a parent, the method fell back to float(self._parent_node.edge.length); if the parent's edge length was also None, this raised a TypeError instead of degrading to 0.0 the way the sibling no-parent/no-length branch already does. Signed-off-by: Mike German <mike@stepsventures.com>
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #234 +/- ##
==========================================
+ Coverage 88.71% 88.81% +0.09%
==========================================
Files 114 114
Lines 12853 12963 +110
==========================================
+ Hits 11403 11513 +110
Misses 1450 1450
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
mmore500
requested changes
Jul 12, 2026
mmore500
left a comment
mmore500
left a comment
Collaborator
There was a problem hiding this comment.
Looks great! Thanks for your work on this @steps-re , think this is a good use of the new capabilities for static analysis that are coming online --- some good catches in here. There is one change I flagged I want to verify further, so if we can split that out into its own PR, I think that will be helpful.
Otherwise, a few very minor suggestions then I think we'll be ready to merge.
steps-re
added a commit
to steps-re/DendroPy
that referenced
this pull request
Jul 13, 2026
…y collapse behavior Applies @mmore500's review on jeetsukumaran#234: - taxonmodel/discrete: accept docstring suggestion edits - drop the SES-shuffle and is_bipartitions_updated deprecation tests (complicated tests for simple fixes, per request); the underlying source fixes stay - replace the collapse_negative_edges crash test with a behavioral test that asserts a negative edge (child older than parent) is actually collapsed by raising the parent to its oldest child's age Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
steps-re
added a commit
to steps-re/DendroPy
that referenced
this pull request
Jul 13, 2026
Per @mmore500's review on jeetsukumaran#234, the change that stopped Node._set_edge() from removing the node from its parent's _child_nodes list needs its own discussion (whether _parent_node should also be reset). Moved to branch fix/node-parent-reindex; removed here along with its regression test. The unrelated distance_from_root() None-parent fix stays in this sweep. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…y collapse behavior Applies @mmore500's review on jeetsukumaran#234: - taxonmodel/discrete: accept docstring suggestion edits - drop the SES-shuffle and is_bipartitions_updated deprecation tests (complicated tests for simple fixes, per request); the underlying source fixes stay - replace the collapse_negative_edges crash test with a behavioral test that asserts a negative edge (child older than parent) is actually collapsed by raising the parent to its oldest child's age
Per @mmore500's review on jeetsukumaran#234, the change that stopped Node._set_edge() from removing the node from its parent's _child_nodes list needs its own discussion (whether _parent_node should also be reset). Moved to branch fix/node-parent-reindex; removed here along with its regression test. The unrelated distance_from_root() None-parent fix stays in this sweep.
mmore500
force-pushed
the
fix/static-analysis-sweep
branch
from
July 13, 2026 22:46
4e4fe7b to
ae2c0f4
Compare
Collaborator
|
Stripped @claude authorship because we haven't yet set a policy for the project (for my own projects, I tend to use tool authorship, but this should probably be decided in a more formal way on a shared project like this). |
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
3 participants
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.
Following up on your note in #233 (and thanks for the quick v5.0.10 release). I ran a static-analysis sweep over DendroPy and verified a handful of small, independent bugs. Each is its own commit with a regression test that fails before the change and passes after.
treemodel/_node.py(Node._set_edge): reassigning a node'sedgesilently dropped it from its parent's child list, corrupting the treecalculate/treesum.py(summarize_node_ages_on_tree):is_bipartitions_updated=Truetriggered a re-encode instead of skipping one, the opposite of every sibling method in the modulecalculate/treesum.py(summarize_node_ages_on_tree):collapse_negative_edges=Truecrashed with aTypeErroron essentially any tree (compared ages in the wrong traversal order)calculate/phylogeneticdistance.py(_calculate_standardized_effect_size): unweighted (edge-count) SES statistics never randomized the null model at all, due to a copy-pasted variable namemodel/discrete.py(simulate_discrete_chars): theroot_statesargument was silently discarded in favor of a random root sequencedatamodel/charmatrixmodel.py(CharacterMatrix.concatenate): the unequal-length-sequences error message showed a literal%dinstead of the matrix indexdatamodel/charmatrixmodel.py(CharacterMatrix.concatenate): concatenating matrices with a shared label hung in an infinite loopdatamodel/charmatrixmodel.py(DiscreteCharacterMatrix.append_taxon_sequence): raisedAttributeErroron any non-empty call (referenced a nonexistent attribute)utility/container.py(NormalizedBitmaskDict): mutating the dict silently broke all iteration (items()/keys()/values()) whilelen()stayed correctdatamodel/taxonmodel.py(Taxon.__init__): constructing aTaxonfrom anotherTaxon(or copying/cloning one) dropped itscommentstreemodel/_node.py(Node.distance_from_root): crashed with aTypeErroron a node with no edge length whose parent also had no edge lengthUp front, so there are no surprises: I found these with AI assistance (a static-analysis pass), and I reviewed, reproduced, and tested each one myself before sending. I dropped a fair number of false positives from the same sweep along the way.
If you would rather I split these into separate PRs, or open issues instead, just say the word. I also have a longer list of candidates from the sweep that I have not fully verified yet. Happy to work through and send fixes for any areas that would be useful to you, or to hold off. Whatever fits how you like to receive contributions.