schema: add KeyPoint3D, KeyLine3D, Polygon3D

[nehalmamgain]

• add 3D equivalents in Cartesian coordinates in sensor space for KeyPoint2D,
  KeyLine2D and Polygon2D on the image plane respectively.

This PR is based on Nisse's PR #45
I don't have write access to the aformentioned PR so I opened a new PR with the changes asked by Quincy, after clarifications from Nisse.

Sorry for the incovenience @tk-woven ; let's continue discussion on this PR.

---

This change is 

[nehalmamgain]

@tk-woven

Could you please share where we can find documentation of keys and value semantics to expect here?

You can see some of the attributes here. Please note that the contents for attributes differ based on the project - linked ones are for lane lines. I do not know which values for the other projects.

[nehalmamgain]

Btw please let me know if you somehow don't have access to the doc (it is in TME drive).

[tk-woven]

+reviewer:@kuanleetri

Reviewable status: 0 of 2 files reviewed, all discussions resolved (waiting on @kuanleetri)

[tk-woven]

Thanks for the link to the doc. I am able to view it!

Let's add some usage instructions for the attributes map in the code. I went into detail in a company-internal Slack exchange about potential issues with that representation which I won't repeat in full here. Adding "usage" docs to the attributes map is a fair middle ground. Basically, we want to discourage people from putting arbitrary data in that map, so the middle ground would be instructing users to only add key/value pairs that are stored in some document accessible by all potential users of the data in their project. This way people parsing the data have a guide on how to interpret values, for example when they need to cast or do bounds-checking.

This doesn't stop anyone from violating the semantics, which is why I call it a middle ground.

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @kuanleetri and @nehalmamgain)

---

dgp/proto/annotations.proto line 251 at r1 (raw file):

// 3D point annotation.
message KeyPoint3DAnnotation {
  // Class identifier (should be in [0, num_classes - 1])

Can you share where we can find the num_classes value please?

---

dgp/proto/annotations.proto line 258 at r1 (raw file):

  // An associative map stores arbitrary attributes, where the key is attribute name
  // and the value is attribute value. Both key_type and value_type are string.

Let's condense this to

// A map of attribute names to their values.

---

dgp/proto/annotations.proto line 261 at r1 (raw file):

  map<string, string> attributes = 3;

  // An identifier key. Used to link with other annotations.

How does one establish a link with this string?

[nehalmamgain]

Understood, thanks! Will make the change

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @kuanleetri, @pd-nisse, and @tk-woven)

---

dgp/proto/annotations.proto line 251 at r1 (raw file):

Previously, tk-woven wrote…

Can you share where we can find the num_classes value please?

I brought this comment over from the previous PR, but basically this value (total number of classes) will differ ontology to ontology.
It's hard to say num_classes is a fixed value.
You would like me to add some comment explaining what's expected in num_classes?

---

dgp/proto/annotations.proto line 258 at r1 (raw file):

Previously, tk-woven wrote…

Let's condense this to

// A map of attribute names to their values.

Gotcha

---

dgp/proto/annotations.proto line 261 at r1 (raw file):

Previously, tk-woven wrote…

How does one establish a link with this string?

I can't seem to, but could you please tag @pd-nisse for this question?

[tk-woven]

Thank you!

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @kuanleetri, @nehalmamgain, @pd-nisse, and @tk-woven)

---

dgp/proto/annotations.proto line 251 at r1 (raw file):

Previously, nehalmamgain wrote…

I brought this comment over from the previous PR, but basically this value (total number of classes) will differ ontology to ontology.
It's hard to say num_classes is a fixed value.
You would like me to add some comment explaining what's expected in num_classes?

Thanks. The way it's styled makes me think we can find num_classes in some message definition or other code. If it differs based on the ontology, let's please note that in the comment, basically something like

// Class identifier (should be in [0, num_classes_in_your_ontology -1]).

---

dgp/proto/annotations.proto line 261 at r1 (raw file):

Previously, nehalmamgain wrote…

I can't seem to, but could you please tag @pd-nisse for this question?

@pd-nisse , could you let us know, please?

[nehalmamgain]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @kuanleetri, @nehalmamgain, @pd-nisse, and @tk-woven)

---

dgp/proto/annotations.proto line 261 at r1 (raw file):

Previously, tk-woven wrote…

@pd-nisse , could you let us know, please?

P.S. While editing the file to factor in your suggestions, I believe this section is just copied over from KeyPoint2D

  // An identifier key. Used to link with other annotations.
  string key = 4;

But yes, let's wait for Nisse to provide more context here.

[nehalmamgain]

Reviewable status: 1 of 2 files reviewed, 3 unresolved discussions (waiting on @kuanleetri, @pd-nisse, and @tk-woven)

---

dgp/proto/annotations.proto line 251 at r1 (raw file):

Previously, tk-woven wrote…

Thanks. The way it's styled makes me think we can find num_classes in some message definition or other code. If it differs based on the ontology, let's please note that in the comment, basically something like

// Class identifier (should be in [0, num_classes_in_your_ontology -1]).

Done.

---

dgp/proto/annotations.proto line 258 at r1 (raw file):

Previously, nehalmamgain wrote…

Gotcha

Done.

[tk-woven]

Thanks, Nehal! Waiting on Nisse's feedback, but otherwise looks good to me :)

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @kuanleetri and @pd-nisse)

[nehalmamgain]

Cool, thanks!

[pd-nisse]

@nehalmamgain @tk-woven : You are correct, the key attribute was simply copied from KeyPoint2DAnnotation, to have an analogous solution.
Since Points/LineAnnotations do not have an instance_id, the attribute key is used to cross-reference them. An example would be different skeleton points. Those can then be referenced on the parent "Pedestrian" BoundingBox2DAnnotation as relating to that specific instance.

[tk-woven]

Thanks Nisse :) I'd like us to add some documentation to the key field about how to determine appropriate values to set the string to. Nehal, are you familiar with the work enough to extend the docs with information on selecting proper values, or do we need further explanation?

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @kuanleetri and @pd-nisse)

[nehalmamgain]

Thanks for the explanation Nisse!

Nehal, are you familiar with the work enough to extend the docs with information on selecting proper values, or do we need further explanation?

I understood what Nisse meant. Having never worked with KeyPoint2D (and not seen the 3D jsons that will be produced after this PR merges), I'm not sure of the proper values but I can look it up.
Since you've worked with the OD DGP datasets before (and they do contain key point 2D information), if you would like to extend the docs yourself, please let me know!

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @kuanleetri and @pd-nisse)

[tk-woven]

Thanks! Personally, I still have very little idea of what valid values of key could be or how to use that field, so I defer to the authors here and people that have put that key to use before.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @kuanleetri and @pd-nisse)

[nehalmamgain]

I still have very little idea of what valid values of key could be or how to use that field,

Me either, I have never used this field.
Let me just investigate an OD dataset since this is still blocking the PR.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @kuanleetri and @pd-nisse)

[nehalmamgain]

@tk-woven There seems to be a string value like "key_point_2d_key": "0fe8ec63-adce-4737-a8db-d39e918c9ce0" for a bounding box instance like a car, and associated key point json has the corresponding "key": "0fe8ec63-adce-4737-a8db-d39e918c9ce0". How do you wish to proceed?

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @kuanleetri and @pd-nisse)

[tk-woven]

Thanks Nehal. So, it seems that all annotations related to some ground truth instance associate themselves with the instance by using the instance's key as a value for their own key field. Let's update the docs to note this, and then I believe we're good to go :)

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @kuanleetri and @pd-nisse)

[tk-woven]

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @kuanleetri)

[tk-woven]

, but it looks like we might have some CI failures unfortunately.

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @kuanleetri)

[nehalmamgain]

Force pushing trivial change (and undoing it) to restart the CI
Basically

.pylintrc:1: [E0015(unrecognized-option), ] Unrecognized option found: accept-no-param-doc, accept-no-return-doc, accept-no-yields-doc

should not be showing up for a *_pb2.py file which is in ignore list.

[nehalmamgain]

Still fails...
@tk-woven I will look to your support for merging this please 🙇‍♀️

[kuanleetri]

Reviewed 1 of 2 files at r1, 1 of 1 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @nehalmamgain)

[kuanleetri]

Pass the test should be good to go.

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @nehalmamgain)

[tk-woven]

Thanks Kuan! Nehal, I'll look into the CI issue today.

[tk-woven]

The problem in CI is that the check_docs pylint plugin is not available, and so the configuration we're providing is invalid. Looking into how to fix this.

[tk-woven]

Fix for CI issue is in #109. It's the only issue I have identified at this time.

[nehalmamgain]

Pass the test should be good to go.

Thanks @kuanleetri
Although this PR was passing the CI test before, it fails out of the blue now. Tyler kindly posted a fix as mentioned. Please review when convenient 🙇‍♀️

[tk-woven]

I'm guessing we picked up a new pylint version with a recent Docker build on master. We could confirm if anyone has an older image laying around, but I don't think it's mandatory since the fix will take care of the issue.

[tk-woven]

#109 is merged, so let's give CI another shot here!

[nehalmamgain]

Thanks again Tyler 🙂