-
Notifications
You must be signed in to change notification settings - Fork 75
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
Add TbV log clustering by scene (i.e. spatial location). #26
Conversation
""" | ||
|
||
|
||
ATX_CLUSTERS = { |
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.
ATX_CLUSTERS = { | |
ATX_CLUSTERS: Final[Dict[int, Tuple[str, ...]]] = { |
My suggestion is to make these map from int to tuples of strs since they're static data.
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.
Good call @benjaminrwilson. Just updated to make this change.
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.
Actually, looks like mypy
interprets 1: ("mystring")
as int: str
instead of int: Tuple[str,...]
.
src/av2/datasets/tbv/scene_clusters.py:20: error: Dict entry 2 has incompatible type "int": "str"; expected "int": "Tuple[str, ...]"
src/av2/datasets/tbv/scene_clusters.py:21: error: Dict entry 3 has incompatible type "int": "str"; expected "int": "Tuple[str, ...]"
src/av2/datasets/tbv/scene_clusters.py:26: error: Dict entry 5 has incompatible type "int": "str"; expected "int": "Tuple[str, ...]"
I think I'll have to revert it.
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.
Wouldn't we need to add a comma for a tuple literal?
E.g., 1: ("mystring",)
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 see, yeah. I think that's a bit uglier though. I would prefer we keep it as list.
PR Summary
Some users are interested in clustering TbV logs by spatial location, to identify before/after logs. This PR adds annotation of which logs are clustered together, in which cities. Each log in each cluster shares some visual overlap.
Testing
In order to ensure this PR works as intended, it is:
Compliance with Standards
As the author, I certify that this PR conforms to the following standards: