Skip to content
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

Merged
merged 2 commits into from
Jul 27, 2023

Conversation

DavidTorresOcana
Copy link
Contributor

@DavidTorresOcana DavidTorresOcana commented Mar 29, 2023

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 Reviewable

Copy link
Collaborator

@rachel-lien rachel-lien left a 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

@DavidTorresOcana
Copy link
Contributor Author

dgp/utils/pose.py line 171 at r2 (raw file):

Previously, rachel-lien wrote…

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

It seems I missed to document these. New commit should address these

rachel-lien
rachel-lien previously approved these changes May 5, 2023
Copy link
Collaborator

@rachel-lien rachel-lien left a comment

Choose a reason for hiding this comment

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

:lgtm:

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

Copy link
Collaborator

@yuta-tsuzuki-woven yuta-tsuzuki-woven left a comment

Choose a reason for hiding this comment

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

:lgtm: with a minor comment

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

@DavidTorresOcana
Copy link
Contributor Author

dgp/utils/pose.py line 92 at r3 (raw file):

Previously, yuta-tsuzuki-woven wrote…

nit, I think you need to add Parameters explanation in the docstring

        Parameters
        ----------
        new_reference_coordinate_system: str
            ...

oh! I missed this! Thank you for catching it.
I also updated the docstring style of that file to match Numpy's

rachel-lien
rachel-lien previously approved these changes May 23, 2023
Copy link
Collaborator

@rachel-lien rachel-lien left a 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: :shipit: complete! all files reviewed, all discussions resolved (waiting on @DavidTorresOcana)

Copy link
Contributor Author

@DavidTorresOcana DavidTorresOcana left a comment

Choose a reason for hiding this comment

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

:lgtm:

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

feat: add reference coordinate system to poses
Copy link
Collaborator

@rachel-lien rachel-lien left a comment

Choose a reason for hiding this comment

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

:lgtm:

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

Copy link
Collaborator

@yuta-tsuzuki-woven yuta-tsuzuki-woven left a comment

Choose a reason for hiding this comment

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

:lgtm:

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

@DavidTorresOcana
Copy link
Contributor Author

is it ok to merge?

Copy link
Collaborator

@yuta-tsuzuki-woven yuta-tsuzuki-woven left a comment

Choose a reason for hiding this comment

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

LGTM

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

@DavidTorresOcana
Copy link
Contributor Author

Anything else to do @rachel-lien @yuta-tsuzuki-woven ?

@yuta-tsuzuki-woven yuta-tsuzuki-woven merged commit a64e360 into TRI-ML:master Jul 27, 2023
3 checks passed
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