Lidar Scene Transforms #265
Conversation
…client into 3D_IngestBranch
|
Random style comments: etc. |
| @@ -0,0 +1,138 @@ | |||
| import numpy as np | |||
There was a problem hiding this comment.
the ordering of these imports is off, we have a pre-commit hook that fixes import oordering automatically, my bet is that you just don't have it installed.
Try running the following in your repo:
poetry run pre-commit install
There was a problem hiding this comment.
Having trouble running this
| for frame in self.frames: | ||
| frame.dev_pose = offset @ frame.dev_pose | ||
|
|
||
| def apply_transforms(self, relative: bool = False): |
There was a problem hiding this comment.
what happens if a user double applies make_transforms_relative? is it bad? if yes, we should prevent them from doing it twice (maybe add a boolean flag to represent if it's already been applied)
There was a problem hiding this comment.
Commited new changes to handle double transformation
| self.make_transforms_relative() | ||
| for frame in self.frames: | ||
| for item in frame.items: | ||
| if isinstance(frame.items[item], RawPointCloud): |
There was a problem hiding this comment.
maybe can get rid of this check, what other class would it be an instance of?
There was a problem hiding this comment.
It could be a CameraCalibration object
| def __add__(self, other): | ||
| return Transform(other) @ self | ||
|
|
||
| def __matmul__(self, other): |
There was a problem hiding this comment.
this would be the work of a 3d engineer
| from pyquaternion import Quaternion | ||
|
|
||
|
|
||
| class Transform: |
There was a problem hiding this comment.
seems like the user can supply a lot of different values in the constructor. Do all class methods apply to all possible types of values? if not, you may want to separate into different class types (maybe use inheritance?)
Co-authored-by: Sasha Harrison <[email protected]>
Co-authored-by: Sasha Harrison <[email protected]>
Co-authored-by: Sasha Harrison <[email protected]>
Co-authored-by: Sasha Harrison <[email protected]>
patrick-scale
requested review from
tetranoir,
ardila,
jean-lucas,
gatli and
pfmark
and removed request for
jean-lucas
| @@ -0,0 +1,43 @@ | |||
| from nucleus.sensorfusion import * | |||
There was a problem hiding this comment.
We do this elsewhere in the code but it's generally not good style, so good to avoid if possible
| @@ -0,0 +1,43 @@ | |||
| from nucleus.sensorfusion import * | |||
There was a problem hiding this comment.
This is close to being a real unit test but
A) The test data you use isn't being checked in
B) There are no automatic checks for expected results (i.e. using assert)
Since this is a first pass, fine to skip for now, just letting you know this is VERY close!
There was a problem hiding this comment.
Ok wait I changed my mind, we need to turn this into a unit test and at least check that it runs. Ok if you don't know how to add checks for the expected results, but somewhere else in the code review I saw we are missing a dependency. If we make this a real unit test, this would have caught that issue.
For an example of a unit test, see any of the files under test/
| @@ -0,0 +1,19 @@ | |||
| import open3d as o3d | |||
There was a problem hiding this comment.
This needs to be added as a project dependency using
poetry add
I think it's as simple as
poetry add open3d
| @@ -0,0 +1,275 @@ | |||
| import numpy as np | |||
| import transforms3d as t3d | |||
There was a problem hiding this comment.
2 more undeclared dependency that needs to be added so when users
pip install scale-nucleus
they won't get an error when running this file.
There was a problem hiding this comment.
I think this is clean, self contained at a high level. Nice job patrick. I really like where you landed with the high level design of this. There are few concepts for users to learn and they appear to be easy to use.
We do need to add unit tests to ensure that it works as expected for our users. For example, right now users would get import errors, because some of the dependencies of this code have not been added to the project.
No description provided.