TRI-ML / dgp

ML Dataset Governance Policy for Autonomous Vehicle Datasets
https://tri-ml.github.io/dgp/
MIT License
93 stars 63 forks source link

schema: add KeyPoint3D, KeyLine3D, Polygon3D #101

Closed nehalmamgain closed 2 years ago

nehalmamgain commented 2 years ago

This PR is based on Nisse's PR https://github.com/TRI-ML/dgp/pull/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 Reviewable

nehalmamgain commented 2 years ago

@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 commented 2 years ago

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

nehalmamgain commented 2 years ago

Cool, thanks!

pd-nisse commented 2 years ago

@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.

nehalmamgain commented 2 years ago

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 commented 2 years ago

Still fails... @tk-woven I will look to your support for merging this please πŸ™‡β€β™€οΈ

tk-woven commented 2 years ago

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

tk-woven commented 2 years ago

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 commented 2 years ago

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

nehalmamgain commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

nehalmamgain commented 2 years ago

Thanks again Tyler πŸ™‚