Lidar Scene Transforms by patrick-scale · Pull Request #265 · scaleapi/nucleus-python-client

patrick-scale · 2022-03-24T00:17:59Z

No description provided.

…client into 3D_IngestBranch

sasha-scale · 2022-04-07T23:16:31Z

Random style comments:
In python, it is conventional to name variables and filenames in snake_case, so I suggest you rename your files to:

IngestDevelopmentScripts/TestScript.py -> ingest_development_dcripts/test_script.py

etc.

sasha-scale · 2022-04-07T23:18:22Z

nucleus/sensorfusion.py

@@ -0,0 +1,138 @@
+import numpy as np


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

Having trouble running this

nucleus/sensorfusion.py

sasha-scale · 2022-04-07T23:30:42Z

nucleus/sensorfusion.py

+        for frame in self.frames:
+            frame.dev_pose = offset @ frame.dev_pose
+
+    def apply_transforms(self, relative: bool = False):


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)

Commited new changes to handle double transformation

sasha-scale · 2022-04-07T23:31:10Z

nucleus/sensorfusion.py

+            self.make_transforms_relative()
+        for frame in self.frames:
+            for item in frame.items:
+                if isinstance(frame.items[item], RawPointCloud):


maybe can get rid of this check, what other class would it be an instance of?

It could be a CameraCalibration object

nucleus/transform.py

sasha-scale · 2022-04-08T01:15:14Z

nucleus/transform.py

+    def __add__(self, other):
+        return Transform(other) @ self
+
+    def __matmul__(self, other):


this would be the work of a 3d engineer

sasha-scale · 2022-04-08T01:32:31Z

nucleus/transform.py

+from pyquaternion import Quaternion
+
+
+class Transform:


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 <70984140+sasha-scale@users.noreply.github.com>

ardila · 2022-04-11T17:04:59Z

ingest_development_scripts/testing_script.py

@@ -0,0 +1,43 @@
+from nucleus.sensorfusion import *


We do this elsewhere in the code but it's generally not good style, so good to avoid if possible

ardila · 2022-04-11T17:06:32Z

ingest_development_scripts/testing_script.py

@@ -0,0 +1,43 @@
+from nucleus.sensorfusion import *


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!

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/

Will check this out!

ardila · 2022-04-11T17:09:35Z

nucleus/sensorfusion_utils.py

@@ -0,0 +1,19 @@
+import open3d as o3d


This needs to be added as a project dependency using

poetry add

I think it's as simple as

poetry add open3d

ardila · 2022-04-11T17:11:57Z

nucleus/transform.py

@@ -0,0 +1,275 @@
+import numpy as np
+import transforms3d as t3d


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.

ardila

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.

patrick-scale added 10 commits March 23, 2022 17:15

adding some files

c284792

Nucleus 3D ingest base level transforms

9b31d95

merging w/ master

84e1eec

updating testing file

5f155ec

cleaning

19c57c6

Sensor Fusion base transforms

479c342

Sensor Fusion Basic Objects

96cacb9

Merge branch '3D_IngestBranch' of github.com:scaleapi/nucleus-python-…

4095a57

…client into 3D_IngestBranch

cleaning2

3cea689

adding utils

26c3578

patrick-scale requested a review from sasha-scale April 5, 2022 21:13

sasha-scale changed the title ~~adding some files~~ Lidar Scene Transforms Apr 7, 2022

sasha-scale reviewed Apr 8, 2022

View reviewed changes

patrick-scale and others added 5 commits April 8, 2022 11:25

Update nucleus/sensorfusion.py

f3c2fa7

Co-authored-by: Sasha Harrison <70984140+sasha-scale@users.noreply.github.com>

Update nucleus/sensorfusion.py

9d9e32d

Co-authored-by: Sasha Harrison <70984140+sasha-scale@users.noreply.github.com>

Update nucleus/sensorfusion.py

5021ce6

Co-authored-by: Sasha Harrison <70984140+sasha-scale@users.noreply.github.com>

Update nucleus/transform.py

4cca2e6

Co-authored-by: Sasha Harrison <70984140+sasha-scale@users.noreply.github.com>

feedback updates

f7c0d39

patrick-scale requested review from ardila, gatli, jean-lucas, pfmark and tetranoir and removed request for jean-lucas April 8, 2022 23:28

exception updates and name changes

203ec13

tetranoir requested a review from jean-lucas April 11, 2022 16:43

ardila reviewed Apr 11, 2022

View reviewed changes

ardila suggested changes Apr 11, 2022

View reviewed changes

		@@ -0,0 +1,275 @@
		import numpy as np
		import transforms3d as t3d

Conversation

patrick-scale commented Mar 24, 2022

Uh oh!

sasha-scale commented Apr 7, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ardila Apr 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ardila left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ardila Apr 11, 2022 •

edited

Loading

ardila left a comment •

edited

Loading