New labelling system#115
Conversation
| data_type: Any = None, | ||
| ) -> None: | ||
| if not data and data_type is None: | ||
| raise ValueError("data_type cant be `None` if data is empty.") |
|
|
||
| def __setitem__(self, key: Any, item: Any) -> None: | ||
| if not isinstance(item, self.data_type): | ||
| raise TypeError(f"All values must be {self.data_type}") |
There was a problem hiding this comment.
Usually you specify which data was the input
| super().__init__(iterable, str) | ||
|
|
||
|
|
||
| class Labelling: |
There was a problem hiding this comment.
Can you do the docstring please?
| l = Labels(value) | ||
| if not value: | ||
| return |
There was a problem hiding this comment.
Is that order correct?
You return nothing and not doing anything, is that on purpose?
| if not set(l).issubset(self._nodes): | ||
| raise ValueError( | ||
| "All ids in the labelset should correspond to ids in the LineageTree object." | ||
| ) |
There was a problem hiding this comment.
Does it create errors if you have keys that are not in the set of nodes?
If yes which ones?
If no why do you protect for it?
|
|
||
| if data_type is None: | ||
| if not isinstance(data, Mapping): | ||
| tmp_d = tuple(data) |
There was a problem hiding this comment.
That's really not optimal, you transform the whole dataset as a tuple to get the first element.
Maybe:
value = next(iter(data))
if len(value) != 2:
raise ValueError(f"`data` could not be converted to dict.")
else:
self.data_type = type(next(iter(data))[1])| def __init__( | ||
| self, | ||
| data: Iterable, | ||
| data_type: Any = None, |
There was a problem hiding this comment.
Aren't you expecting a type rather than Any?
| def get_norm(self, root) -> int: | ||
| return len( | ||
| self.lT.get_all_chains_of_subtree(root, end_time=self.end_time) | ||
| ) | ||
| # def get_norm(self, root) -> int: | ||
| # return len( | ||
| # self.lT.get_all_chains_of_subtree(root, end_time=self.end_time) | ||
| # ) | ||
|
|
||
| assert lt.unordered_tree_edit_distance( | ||
| 176, 29345, style=new_tree | ||
| ) == lt.unordered_tree_edit_distance(176, 29345, style="normalized_simple") | ||
| # assert lt.unordered_tree_edit_distance( | ||
| # 176, 29345, style=new_tree | ||
| # ) == lt.unordered_tree_edit_distance(176, 29345, style="normalized_simple") |
There was a problem hiding this comment.
the uted pr is gonna be applied soon, so I let them there to remember to reimplement them after uted (Uted needs this PR to become final)
|
|
||
| @property | ||
| def list_of_labels(self): | ||
| return [k for k, v in self.__dict__.items() if isinstance(v, Labels)] |
There was a problem hiding this comment.
I don't like the idea of a default dict but if you have one I think you should have something like:
def __getitem__(self, key):
return self.default_dict[key]There was a problem hiding this comment.
interesting idea lets discuss that
|
|
||
| class Labelling: | ||
|
|
||
| default_dict = {} |
|
|
||
|
|
||
| class StaticTypedValueDict(UserDict): | ||
| """Dict that allows only one type of values.""" |
There was a problem hiding this comment.
Also, why is this declared in a _labelling.py file? Won't it be used for other types of data?
| self, | ||
| data: Iterable, | ||
| data_type: Any = None, | ||
| ) -> None: |
There was a problem hiding this comment.
I think it would be nice if you also could specify a default return value.
Like for labels it could be "".
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v4.x #115 +/- ##
==========================================
- Coverage 87.16% 83.41% -3.75%
==========================================
Files 20 22 +2
Lines 1932 1960 +28
==========================================
- Hits 1684 1635 -49
- Misses 248 325 +77 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
2 participants
Labelsare now a new class that works like a dict, but tests if all values of the dictionary are actually strings, can be labels. These labels are saved inside a new attribute of LineageTree calledlabelling.lT.labelling contains all the labels and tests if all new labels correspond to a subset of the nodes or the set of the nodes.
To access these labels:
get_ancestor_with_attributeworks the same way as before.get_labelled_ancestorworks the same way as before.I am open to discuss if we should drop labelling and save everything directly on LineageTree.