-
Notifications
You must be signed in to change notification settings - Fork 62
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
schema: add reference coordinate system to Poses #147
schema: add reference coordinate system to Poses #147
Conversation
f335118
to
16d7f71
Compare
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.
+@yuta-tsuzuki-woven Can you help review?
Reviewed 2 of 2 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @DavidTorresOcana and @yuta-tsuzuki-woven)
dgp/utils/pose.py
line 87 at r2 (raw file):
"""Returns a new Pose that corresponds to the inverse of this one.
nit, can you update the docstring with a parameters section for new_reference_coordinate_system
per style guide?
https://github.com/TRI-ML/dgp/blob/master/docs/CONTRIBUTING.md#code-style
https://numpydoc.readthedocs.io/en/latest/format.html#parameters
dgp/utils/pose.py
line 152 at r2 (raw file):
transformation_matrix: np.ndarray 4x4 containing rotation/translation
nit, can you update the docstring parameters section for reference_coordinate_system
per style guide?
https://github.com/TRI-ML/dgp/blob/master/docs/CONTRIBUTING.md#code-style
https://numpydoc.readthedocs.io/en/latest/format.html#parameters
dgp/utils/pose.py
line 171 at r2 (raw file):
tvec : np.ndarray length-3 translation vector """
nit, can you update the docstring parameters section for reference_coordinate_system
per style guide?
https://github.com/TRI-ML/dgp/blob/master/docs/CONTRIBUTING.md#code-style
https://numpydoc.readthedocs.io/en/latest/format.html#parameters
Previously, rachel-lien wrote…
It seems I missed to document these. New commit should address these |
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.
Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @yuta-tsuzuki-woven)
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.
Reviewed all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @DavidTorresOcana)
dgp/utils/pose.py
line 92 at r3 (raw file):
result: Pose new_reference_coordinate_system pose reference_coordinate_system: str
nit, I think you need to add Parameters
explanation in the docstring
Parameters
----------
new_reference_coordinate_system: str
...
74550bb
e3f91d2
to
74550bb
Compare
Previously, yuta-tsuzuki-woven wrote…
oh! I missed this! Thank you for catching 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.
Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @DavidTorresOcana)
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.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @DavidTorresOcana)
74550bb
to
62649be
Compare
feat: add reference coordinate system to poses
62649be
to
a01d344
Compare
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.
Reviewable status: 2 of 3 files reviewed, all discussions resolved (waiting on @DavidTorresOcana)
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.
Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @DavidTorresOcana)
is it ok to merge? |
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.
LGTM
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @DavidTorresOcana)
Anything else to do @rachel-lien @yuta-tsuzuki-woven ? |
Currently, Poses are used to represent SE3 Transformations. A transformation is an affine transformation between two coordinate systems (CS) and, and as such, is bounded to these two (source and destination) coordinate systems.
In other words, a Pose shall indicate the two coordinate system it transforms points in between of.
As, in DGP, each Pose is linked to a datum, a sensor or a piece of information, the only missing CS is the reference (destination) coordinate system.
This PR proposes explicit indication of such Reference Coordinate System on each Pose.
This is in line with ASAM OpenLABEL CS definitions where each piece of information is linked to a CS.
Pending still TODO, but not in the scope of this PR (conversation is encouraged), is how to define and name these CS. ASAM OpenLABEL CS definitions, ISO 8855 and ISO 23150 are a good start.
@rachel-lien @akira-wakatsuki-woven ping
This change is