Replacement date-aware tree-build pipeline - #143
Open
lentinj wants to merge 62 commits into
Open
Conversation
Without -vvv means no output, which isn't what you were hoping for.
Generate node_ages.json using chronosynth.
We need loose package requirements so we don't clash with dated-complete-tree, but should still be installing known versions. Use requirements.txt to achieve this. requirements: peyotl changes merged
Member
|
Excellent. Yes, it would be great to get rid of the badly-hacked shark portion of the tree with something more up-to-date. |
…ing.py Since we don't let dated-complete-tree do it's own filtering, copy it here.
The opentree is going to go via. dated-complete-tree and the pipeline will assume it's dated. To achieve this, fetch the files it needs rather than what the old pipeline needed.
We nneed the tree before we've dated it, and can't remove nonspecies / unary nodes at this point. Leave the rest of the code as-is to borrow it later.
Start reimplementing new pipeline, fully ete4-based. Re-implement pruning/grafting in memory using ete4, so bespoke trees / OpenTree can have date properties. utilities/ete: Generic OTT-extractor function Keep returning output as str. It's a bit confusing, but is generally what we're dealing with one way or another so saves a bunch of conversion.
Lift and shift code from CSV_base_table_creator into a separate file that generates taxon mapping & raw popularity based purely on OT taxonomy.tsv, rather than reading the tree. This may process more nodes than required, but I'm sure we'll manage. We also no longer try and process mrca nodes, but given we explicitly filter them anyway, this has been broken for a long time. References: #131
Import csv generated by taxon_map, and distribute references by OTT about the tree.
Given raw_popularity from the taxon_map, lift-and-shift popularity percolation code out of CSV_base_table_creator into step_popularity.
Note that unlike previously, we don't shuffle the children before ladderizing. However we did so with a fixed seed so I'm not sure the difference is worth the re-implementation.
Use dated-complete-tree where we can, otherwise implement our own in step_tidy
Copy over popularity_add_rank, convert to ete4, add to the pipeline.
As with CSV_base_table_creator, add a ladderizing step.
Copy code to write CSVs from dendropy_extras. Don't bother with the non-MySQL CSVs, go straight for the files we use.
Functions to populate required algorithms into tree properties.
Write an ete4 property out to a data file, using the same sorting as ordered leaves/nodes.
Given a dict, output a JS source file that defines each entry as a variable. Will be used for the cutmap & brief newick.
ete4 versions of make_js_treefiles' JS output conversions. Output native data structures so we can add the conversions later (and potentially remove them).
dated-complete-tree expects everything to have a date property, even if it's None. Stop removing them here.
dated-complete-tree expects everything to have a date property, even if it's None. Stop removing them here.
Hook up step_jsnewick into main pipeline
There's probably a deeper bug, but gets us going for now.
Generate prop arrays in main pipeline
Work through tree fixing cases it gets confused by, and get it running at least.
This has been superseded by tree_build/step_graft, which performs the splicing on an in-memory ete4 tree.
enumerate_one_zoom_tokens was only imported by build_oz_tree and get_open_trees_from_one_zoom, both removed in the preceding commits. Its own dependency, token_to_oz_tree_file_mapping, is still used by tree_build/step_parse and stays.
The original option wasn't psased through to popularity_add_prop, put it back.
It got lost along the way, add the option back and read in checks here too.
Without, previously set data would get lost.
The issue is seemingly solved in latest OT versions, and only using silva NCBI IDs when desperate is losing us a significant number of matches. In addition, the regex only matches a particular ordering of arguments, which generally not matching anyway.
Claude noticed that along with the entries we're expecting, there's a lot of junk in the sources we find, likely upstream parsing problems bubbling up. Whilst removing them doesn't make a dramatic difference practically, it does make the output easier to read.
With at least the current OpenTree, ifung is actually if: https://github.com/OpenTreeOfLife/reference-taxonomy/wiki/Interim-taxonomy-file-format https://tree.opentreeoflife.org/taxonomy/browse?id=75257 https://tree.opentreeoflife.org/taxonomy/browse?id=199764 Use the correct column name up until the DB, where it's not worth the faff of renaming.
It's now been replaced with taxon_map, tidy up old code.
As well as date_tree, ete4 will also clobber logging settings. Force logging options, and restore the error_handler so scripts still stop properly.
lentinj
force-pushed
the
issue-131-new-date-pipeline-2
branch
from
August 27, 2026 10:26
5f8bdfb to
f383150
Compare
Avoid re-runs by splicing the cmd changes into the lockfile.
treeprop_geological read node.props["age"], but nothing in the pipeline ever sets that prop, so every node logged "has no age property" and was assigned geological=0. The function was ported from a DendroPy implementation whose docstring correctly said "date" while its code read node.age -- an attribute the old pipeline really did populate, via dendropy_extras.set_node_ages(). The ete4 port kept the "age" spelling and reconciled the docstring the wrong way; set_node_ages() then left with CSV_base_table_creator. "date" is the name the rest of the pipeline uses, so read that. The tests passed only because their helper synthesised the missing prop. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
impute_missing_dates documents that it "Assumes root node is dated", and no bespoke tree dates biota itself -- tidy_infill_dates_bottomup cannot reach the root, as it has no dist and its Eubacteria child contributes date 0. So the root needs an explicit date or imputation fails outright. The value was 4567, the age of the solar system, which is ~1400My older than the tree's own oldest date (3200) and drags every undated node near the root older with it. 4000 is a Last Universal Common Ancestor estimate, which is what the root actually represents. Use setdefault rather than add_prop so a date supplied by the data is no longer silently overwritten. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
step_output and step_jsnewick treated dist == 0 as the marker for an artificially resolved polytomy. That was never reliable and is about to become wrong: branch lengths are regenerated from dates in a following commit, which overwrites the zeros. It was already missing the OT trees entirely. Their polytomies are resolved upstream by tree_fixing.fix_polytomy, and date_tree writes the tree with no branch lengths at all, so those nodes arrive with dist None -- 965,498 of them in the current dated_tree_pre.tre, every one of them written out as a genuine parent. Record the resolution as a "polytomy" prop instead. Its value names how the topology was chosen, as the two stages differ: ete4's resolve_polytomy is deterministic and pairs children off in order, so every polytomy becomes the same comb, while fix_polytomy draws uniformly at random from the possible topologies. OT nodes are marked by the "mrcapoly" name fix_polytomy gives them, that being all that survives into the newick. Consumers only test the prop for truth, so both kinds work unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The mrcaimp detach ran inside the loop after delete_one_child_nodes, so
every leaf it removed left its parent with a single child and nothing
came along afterwards to collapse it:
after delete_one_child_nodes: ((A:1,mrcaimp:1)X:1,B:1);
after mrcaimp detach : ((A:1)X:1,B:1); <- X now unary
mrcaimp appears 1,084,269 times in the current dated tree, so this was
not a corner case, and it defeated the point of the preceding step.
Do the detach first, in its own pass. That makes it possible for the
root itself to end up unary, in which case delete_one_child_nodes
replaces it -- so assign its return value, which was being discarded.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
tidy_infill_dates_bottomup converts branch lengths into dates and the
pipeline treats 'date' as canonical from then on, but nothing ever
recomputed 'dist' from the result. So branch lengths were whatever the
input newicks happened to carry, before dates were cleared, infilled and
imputed -- and simply absent for nodes whose newick had no length, since
ete4 leaves those as None rather than defaulting to 1.
On a tree run through the real steps, dist disagreed with the dates it
was supposed to describe by more than an order of magnitude:
node date dist parent.date - date
x 10.0 100.0 4557.0
c 0 None 2283.5
treeprop_sliding_window and treeprop_weighted_mean consume dist, so both
were computed from lengths unrelated to the tree's own dates.
compute_branch_lengths exists for exactly this and is called by
date_tree and every dated-complete-tree main, just never here.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
OTT frequently holds one taxon twice, split across source taxonomies -- Q26285047 is OTT 988455 (ncbi + gbif ids) and OTT 4018224 (worms + irmng ids), same genus, same rank. Both rows carry the same raw_popularity, so summing them unaltered counted that popularity once per copy. The code noticed and warned about this but did nothing about it. In the assembled tree 2,473 Qids sit on more than one node, 2,721 nodes in total, and every group shares an identical raw_popularity. They are not duplicates in the sense of being adjacent: only 96 of 2,473 groups are siblings, the median leaf/leaf pair is 73 leaves apart and 292 pairs are separated by over 10,000 leaves. That scatter rules out crediting one copy and zeroing the others -- it would hand a genus's entire popularity to one of two distant clades on an arbitrary basis. Divide it between them instead, which keeps the tree's total right without choosing. Only nodes that actually contribute are counted, so an excluded or unpopulated duplicate does not dilute its twin. Rebuilding, 4.59% of leaves change popularity. popularity_rank changes for 99% of leaves, but 94.56% of those have an unchanged popularity and zero order inversions between them -- competition ranking renumbers everything below any changed value. The 15 most popular leaves are unchanged. The per-node warning is now inaccurate, as the double-counting no longer happens; it moves to debug with a single summary warning in its place. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
graft_extract_ot_subtrees detaches each requested subtree from the
OpenTree tree in place, but left behind any ancestor that detaching
emptied. An emptied node has no children, so everything downstream reads
it as a leaf: it was written to ordered_leaves.csv as though it were a
species, with no OTT but a real popularity and leaf rank, and
date_labelling warned that a leaf had a non-zero date.
BonyFishOpenTree.PHY requests every family under Beryciformes
individually and arranges them itself, so the whole OT clade is
relocated and its scaffolding hollowed out:
Beryciformes_ott587933 (128 leaves in OT)
|- mrcaott90320ott98190
| |- Cetomimidae_ott118790 <- requested
| `- mrcaimp -> Melamphaidae, Berycidae, Gibberichthyidae <- requested
`- mrcaott118778ott325745
|- mrcaott118778ott781203 -> Barbourisiidae, Stephanoberycidae <- req.
`- Rondeletiidae_ott190706 <- requested
Prune at the point of detachment, walking up while nodes are left empty.
Only nodes emptied by us are removed -- a node that was already a tip is
a real taxon and is left alone -- and the root is never detached.
Doing this by name after the fact does not work: nothing in the final
tree distinguishes an emptied taxon from a genuine leaf, and pruning
just the synthetic mrca* nodes merely moves the problem up to
Beryciformes itself. tidy_prune_synthetic_leaves still exists for
mrcaimp debris that arrives in the source data independently of
extraction, and replaces the narrower mrcaimp pass in tree_build.
Rebuilding: both warnings gone with no replacement, and four fewer
leaves (2287173 -> 2287169) -- the three mrca* nodes plus Beryciformes.
The no-OTT leaf count drops to exactly the 2051 genuine unplaced taxa,
so nothing real was pruned.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
They ended up being redundant, only geological is used by the renderer in #1013.
read_taxon_map() gives every taxon a full set of keys, using None for the
columns that were empty, so .get(key, "\N") never fired and csv.writer
wrote the None as an empty string. Add mysql_null() and use it for every
taxon-derived and node column.
Whilst here:
* Read the taxon map's "rank" column, rather than the DB's name for it
("rnk"), which meant the rank was never exported at all.
* Write the unpopulated "price" / "vern_synth" columns as \N too.
lentinj
force-pushed
the
issue-131-new-date-pipeline-2
branch
from
August 28, 2026 15:07
bec5b99 to
2d2823d
Compare
We don't know the tree serial here, so can't fill it. Using \\N doesn't work, since the column is NOT NULL. Put a placeholder value to get replaced later by the import script.
lentinj
force-pushed
the
issue-131-new-date-pipeline-2
branch
from
August 28, 2026 16:00
2d2823d to
d0463f4
Compare
lentinj
marked this pull request as ready for review
August 28, 2026 16:18
Collaborator
Author
|
Right, so I stopped trying to be a puritan over the dvc.lock file and just bodged it to not update the stages that I wasn't really working with. But there was clearly a whole bundle of things wrong, I must have lost some commits from my previous branch. And once I started poking other things came out the woodwork @hyanwong it'd be worth you reading through the Claud-y commit messages. I'm happy with the code itself, but:
Finally, problems I still know about:
|
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
2 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.
Replaces the core of the pipeline, i.e. get_open_trees_from_one_zoom, build_oz_tree, CSV_base_table_creator, make_js_treefiles with:
step_graft.py,step_jsnewick.py,step_output.py,step_parse.py,step_popularity.py,step_taxon.py,step_tidy.py,step_treeprop.pywhich do roughly what they say on the tinCompared to the previous issue-131-new-date-pipeline there's a bunch of tidy-up, and the old scripts that got replaced have been removed.
Also somewhat unrelated, Claude noticed that:
ncbi_silvaspecial-case was largely pointless nowadaysifungsource is actually calledifnowadays. I've not renamed the DB column, but use the new name otherwiseThe main thing left to look at is #130, and try building something with the replacement Sharks tree.
EDIT: And also run the thing and update
dvc.lock. Minor details :)Fixes #131