Skip to content

initial key point support and annotators#1128

Merged
SkalskiP merged 28 commits intodevelopfrom
feat/dev/keypoints-annotated
Apr 24, 2024
Merged

initial key point support and annotators#1128
SkalskiP merged 28 commits intodevelopfrom
feat/dev/keypoints-annotated

Conversation

@SkalskiP
Copy link
Collaborator

@SkalskiP SkalskiP commented Apr 23, 2024

Description

  • core KeyPoints class along with from_ultralytics connector
  • VertexAnnotator and SkeletonAnnotator

def annotate(self, scene: ImageType, keypoints: KeyPoints) -> ImageType:
if len(keypoints) == 0:
return scene
if keypoints.class_id is None:
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need class_id at this point?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no use for it, and I doubt anyone would need it in this form.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's drop it for now.

pass


class VertexAnnotator(BaseKeyPointAnnotator):
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I renamed the annotator to VertexAnnotator. In general, I'd like to use the vertex and edge naming conventions.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good. At the back of our minds, let's also start thinking of names for triplets (joints?)

Having those will let us compute angles, compare skeletons and create new annotators.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we change SkeletonAnnotator to EdgeAnnotator too?

for xy in xy_all:
skeleton = self.skeleton
if not skeleton:
skeleton = resolve_skeleton_by_vertex_count(len(xy))
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I simplified skeleton resolving logic here.

@LinasKo
Copy link
Contributor

LinasKo commented Apr 24, 2024

(This is a comment about skeleton changes - it got too broad to have inline 🙂 )

Upon sleeping on it, I'd have dropped the singleton but kept the logic as a static class.

Some thoughts about your changes:

I like that:

  1. It's much more concise
  2. It's stateless
  3. It digs up the fact that it's a COCO skelly.
  4. It adds the by-vertex resolution method

I don't like that:

  1. It does an O(n) search over all skellies and loops over every vertex every time. I suggest treating skeletons as immutable going forward and doing as little recomputation as possible, so having state (dict or @lru_cache would be better).
  2. Annotators now won't check for None skeletons and crash. I don't like that. I think they should work every time - let's decide on which one's better:
    • Annotator draws no edges + prints warning
    • Annotator draws no edges
    • Annotator links neighbouring links consecutive classes
  3. Optional. I'd want to get rid of null-types as much as possible. They're a vector for bugs, especially if folks aren't using mypy.

Having a dict instead of resolve_skeleton_by_vertex_count would let us avoid recomputation and provide a get method which I see as marginally better than returning None from a function.

I'm neutral about:

  1. I'm still thinking about iteration over Skeleton enum. Yes, it's an enum, so it makes sense. No, it's singular, so intuitively it feels like for edge in Skeleton should work.
  2. I wanted something like static-assertions for devs adding skeletons. Simple to implement, and every time I do, it saves my ass eventually (hence the whole add_skeleton method checking if something with X limbs already exists).
  3. Later we'll want to compute joint angles, count scores based on criteria, compare two skeletons.
    • Simpler is better (praise YAGNI), but we know these will be needed soon.
    • This can be done as a combination of: expanded KeyPoints class, an expanded Skeletons class, a new Joints class (3-vertex tuples), lists of tuples, external non-member functions.

At the end of the day, if we're not using static checks anyway, I'm leaning towards this:

  • Skeleton, defined as a class or List[Tuple[int, int]]
  • A dict or two for predefined skeletons: vertex / edge count -> Skeleton
  • Do we want an enum? Let's make one after we've defined everything else. It's a convenience at this point, rather than a core datatype.
  • Let's fix annotator crashing when no skeleton is found. (for class_a, class_b in skeleton:)

When you read this, let's chat and wrap this one up 🙂

P.S. I'll see if I can add Mediapipe so it informs our decision a bit more. Maybe let's add from_inference too?

@SkalskiP
Copy link
Collaborator Author

  1. Let's add @lru_cache on resolve_skeleton_by_vertex_count and resolve_skeleton_by_edge_count.

@SkalskiP SkalskiP marked this pull request as ready for review April 24, 2024 10:21
@SkalskiP SkalskiP merged commit 80e5413 into develop Apr 24, 2024
@Borda Borda deleted the feat/dev/keypoints-annotated branch January 7, 2026 18:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants