export multiple trees

[jameshadfield]

See commit messages for details

• Checks pass
• If making user-facing changes, add a message in CHANGES.md summarizing the changes in this PR

[victorlin]

Not done reviewing but sending this in before going off to lunch

[victorlin]

Suggestion: do this in generate_newick directly. I just tested locally and something like this seems to work:

- echo "($(generate_newick $((n-1))),$prefix$n)"
+ echo "($prefix$n,$(generate_newick $((n-1)))):INTERNAL_$n"

This runs much faster than augur refine, especially for the large tree.

[jameshadfield]

Without running refine we fail validation due to the absence of any divergence values on the tree.

I just tested locally and something like this seems to work:

Do you have a patch which passes the tests?

[victorlin]

I would skip validation since validity should be tested elsewhere. Patch: d1c3997

[victorlin]

Use repr()/!r instead of manual single quotes

Suggested change

	", ".join([f"'{v}'" for v in dups[0:5]]) + ("..." if len(dups)>5 else ""))
	", ".join([f"{v!r}" for v in dups[0:5]]) + ("..." if len(dups)>5 else ""))

Also suggested elsewhere:

• Measurements command #879 (comment)
• filter: Add --empty-results-reporting={error,warn,skip} option #1175 (comment)

[victorlin]

Adopt #1445 here for consistency:

Suggested change

	required.add_argument('--tree','-t', metavar="newick", nargs='+', required=True, help="Phylogenetic tree(s), usually output from `augur refine`")
	required.add_argument('--output', metavar="JSON", required=True, help="Output file (typically for visualisation in auspice)")
	required.add_argument('--tree','-t', metavar="newick", nargs='+', action='extend', required=True, help="Phylogenetic tree(s), usually output from `augur refine`")
	required.add_argument('--output', metavar="JSON", required=True, help="Output file (typically for visualisation in auspice)")

[victorlin]

Why not always use a list of trees here, even if it's a one-item list? This would simplify internal logic by avoiding the need to check data type on every reference.

Having multiple trees under the singular key tree is also a bit confusing. Would it be too much to put this in a new plural key trees that's more self-descriptive?

Auspice could continue to support a single tree object for backwards compatibility.

[jameshadfield]

In terms of the final dataset JSON, I'd prefer to keep the single-tree export as it currently is to remain consistent with output many users have become familiar with (and many people have written scripts to expect, as it's the case in nearly all situations).

For the multiple trees export it may help to conceptually think of these as subtrees. The schema has supported an array of (sub)trees since #851 and while trees may be nicer in some regards I don't think it's worth changing the schema here.

Why not always use a list of trees here, even if it's a one-item list? This would simplify internal logic by avoiding the need to check data type on every reference.

In terms of internal implementation, we could use an array which would reduce the conditional logic in the code, and then essentially run line 1236 at the very end to export a single tree object where applicable. I felt this changing of types was more confusing to developers, but 🤷

[victorlin]

Gotcha, makes sense to keep the final JSON as-is.

For internal implementation, I find it confusing when a variable takes on two types especially in an untyped language. Thinking in terms of subtrees, a more concrete suggestion would be to rename internal variables to always assume a list of subtrees instead of tree which is confusingly one or multiple trees. Then at the very end export dynamically based on if it's a singular tree or multiple subtrees. Does that make sense?

I've not worked much on this part of the code, so take my comments with a grain of salt!

[jameshadfield]

I can appreciate that maintaining multiple types without any type-hints (which AFAIK is the best python can do) requires developer overhead and won't scale. I chose this pattern as it matches the pattern already in the codebase:

augur/augur/export_v2.py

Lines 113 to 117 in 03ed408

	if isinstance(od.get("tree"), list):
	for subtree in od['tree']:
	order_nodes(subtree)
	elif isinstance(od.get("tree"), dict):
	order_nodes(od['tree'])

but that doesn't prevent us from improving.

a more concrete suggestion would be to rename internal variables to always assume a list of subtrees

I think this would be more confusing if we continue to use data_json['tree'] (unsure if that's what you're suggesting), but I've implemented this using the new variable trees. Could you check out c40b821?

[victorlin]

I see, sorry I should've tried this out first. That doesn't seem any better since we'd still have to keep conditions elsewhere for data_json['tree'] such as

augur/augur/validate_export.py

Lines 21 to 25 in c40b821

	if isinstance(tree, list):
	for subtree in tree:
	recurse(subtree)
	else:
	recurse(tree)

I'm fine with dropping c40b821 and keeping how you had it before.

[victorlin]

without any type-hints (which AFAIK is the best python can do)

We could use something like the following TypedDict for data_json, but enforcement (#1246) and usefulness across text editors is another story.

Tree = Dict[str, Any]

class DataJson(TypedDict):
    tree: Tree or List[Tree]
    meta: ...