-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Simple aggregation redux #13
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This all seems reasonable to me. Huge improvement; lots of things are simplifed! Could be merged as-is, I think, although I haven't run the tests yet (on a plane!)
I have a bunch of little comments about architecture. Let me know what you think is in/out of scope and I can draft issues.
I think I'm still not entirely pleased with how composability (ie serial aggregation) works, but I don't know yet exactly how I think that should work in practice.
from .utils import table | ||
|
||
|
||
class Aggregation(dict[Taxon, Taxon]): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused why we need both concepts "aggregation" and "taxon map"?
A taxon map could be a dictionary, which guarantees unique keys (the second of the two checks).
It feels like the first check (that the input taxa match the keys in the taxon map) is functionality that should live in the aggregator. The aggregator gets handed input taxa, and so it should check that it's outputting a taxon map that has all those input taxa as keys.
Right now it seems like the aggregator has to hand the input taxa and the taxon map to the aggregation. If the aggregator does one of those checks, then the taxon map can be just a map, with no further functionality?
return {k.name: v.name for k, v in self.items()} | ||
|
||
|
||
class Aggregator(ABC): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per my comment above, I could see a static function that validates that an output taxon map has all the input taxa in it
Then you could count on code writers to know that their .aggregate()
methods should call .validate_map()
before returning the map.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want some minimal validation automatically. Every taxon should be mapped exactly once is the minimum correctness standard, though some Aggregator
s might know more about what to check.
The choices seemed to be what I did (make the map check) or have Aggregator.aggregate()
be a non-abstract method calling some actual aggregation function Aggregator._aggregate()
and then Aggregator.validate()
. Neither is ideal, but I'm open to being told one is less awkward than the other.
I'm also going to dispute the above "then the taxon map can be just a map, with no further functionality." Exporting the Taxon : Taxon
dictto a str : str
dict is going to be one of the most common usage patterns for the existence of this class, so it might as well be a method. Taxon
objects are our internal representations, but everywhere else a user is going to need to interact with taxa, they'll be strings.
) | ||
|
||
|
||
class BasicPhylogeneticAggregator(Aggregator): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally I would prefer just "PhylogeneticAggregator," because I can imagine some more "basic" forms, as per my comments about composability below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name can change, but we have to be able to clearly distinguish between this implicitly fixed-map "put these in those" phylogenetic aggregation and the (probably multiple different) dynamic "put these in something" aggregator(s) that we want to make. My instinct is to qualify all of them (this one being "basic" or "fixed" or something of the sort), but I could be convinced that only the more complex ones need more explicit names.
(This PR is mostly a synthesis of feedback from #11 and #12.)
This PR implements an interface for lineage aggregation, and some simple forms of lineage aggregation.
An
Aggregator
is an object that aggregates (viaAggregator.aggregate(input_taxa)
) and produces anAggregation
.An
Aggregation
is a subclasseddict[Taxon, Taxon]
which gives a mapping from input/unaggregated taxa to some aggregated taxa. It does some limited correctness checking upon initialization and is capable of converting itself to adict[str, str]
(the anticipated use-case being bypolars
' orpandas
'.replace()
to aggregate line-list lineage data).The user-facing
Aggregator
s added are:BasicPhylogeneticAggregator
, which uses aPhylogeneticTaxonomyScheme
to perform taxonomic aggregation. This allows us to, for example, "group all children ofLINEAGE.42
intoLINEAGE.42
," or "group all children ofLINEAGE.42
intoLINEAGE.42
exceptLINEAGE.42.47
and its children which are grouped intoLINEAGE.42.47
." The order of operations is defined by the ordering of the aggregation targets provided. The default assumption is if the user provides bothLINEAGE.42.47
andLINEAGE.42
, they intend a paraphyleticLINEAGE.42
(the "except" as above).ArbitraryAggregator
, which enables the user to specify exactly the mapping they want for all taxa. (That is to say, functionally it does little.) This would, for example, enable grouping a set of COVID lineages into the non-phylogenetic units "SGTF" and "non-SGTF."SerialAggregator
which takes in a set of aggregators and applies them in order. It is on the user to know whether this is a good idea or not. Putting anArbitraryAggregator
before aBasicPhylogeneticAggregator
could easily lead to theBasicPhylogeneticAggregator
doing nothing if none of the arbitrary taxa are found on the taxonomy tree.New tests have been added for these aggregators via
pytest
.The behind-the-scenes
Aggregator
s are:SelfAggregator
, which aggregates every taxon to itself.HomogenousAggregator
, which aggregates every taxon into a single aggregation target.BasicPhylogeneticAggregator
ExampleMisc.
The handling of taxon ordering in
BasicPhylogeneticAggregator
is outsourced to the newsort_taxa()
function, which sorts taxa such that larger taxa get larger indices. For example, we would sort such that we get[JN.1.1.1, JN.1.1, JN.1]
. The ordering is not perfectly defined, e.g. either[JN.1.1.1, JN.1.1, JN.1.2, JN.1]
or[JN.1.1.1, JN.1.2, JN.1.1, JN.1]
would be equally valid.A bug in
PhylogeneticTaxonomyScheme.contains(x, y)
was failing when x was the root, saying that the root did not contain any taxa. This has been fixed.