RSDK-1870 Data Loss in ReadPCDtoBasicOctree #1973
Conversation
viambot
added
viam-org
viambot
added
safe to test
|
Warning changing any of the following functions will break code samples on app if an api for these function changes please contact the fleet team
|
| test.That(t, bOct.meta.TotalY(), test.ShouldAlmostEqual, metadata.TotalY()) | ||
| test.That(t, bOct.meta.TotalZ(), test.ShouldAlmostEqual, metadata.TotalZ()) | ||
|
|
||
| tol := 0.0001 |
There was a problem hiding this comment.
tolerance value, ill update the variable to make that more explicit
There was a problem hiding this comment.
Could you please also add a comment why this is needed?
pointcloud/basic_octree_test.go
Outdated
| @@ -557,8 +557,19 @@ func TestBasicOctreePointcloudIngestion(t *testing.T) { | |||
|
|
|||
| // Test the functionalities involved with converting a pcd into a basic octree. | |||
| func TestReadBasicOctreeFromPCD(t *testing.T) { | |||
| artifactPath := "pointcloud/test_short.pcd" | |||
| t.Run("reading from binary PCD to octree", func(t *testing.T) { | |||
| binaryArtifactPath := "slam/example_cartographer_outputs/viam-office-02-22-1/pointcloud/pointcloud_15.pcd" | |||
There was a problem hiding this comment.
- Thanks for calling out that this takes 15 seconds.
- Why does this take 15 seconds?
- Is it 15 seconds just to get an octree or is it the iterate function that is slow?
- If its the iterate function that is slow, can this be parallelized using the Iterate method parameters to complete within a second?
- Is 15 seconds for doing what this is doing expected behavior & acceptable to consider Map Representation complerte?
There was a problem hiding this comment.
In the additional details section, I note that the actual creation is only 0.3secs. I will look into on parallelizing the iterate calls
There was a problem hiding this comment.
Note: the time for octree/pointcloud creations includes adding all the points
There was a problem hiding this comment.
Can you find out what is taking 15s now? I want to make sure that Iterate() isn't slower than expected.
There was a problem hiding this comment.
It is purely in the validateBasicOctree function. We do approx. 15x checks per node which in a balanced octree is 2*(numPoints) - 1. In addition for each node we construct a new metadata object for comparison which involves iterating through all the points below it in the tree.
If this iterate that happens at every node is removed, the time goes down to 7 secs instead of 15 secs
There was a problem hiding this comment.
@nicksanford based on the time sink being in the validate and caused not by the iterate, I don't think it's worth doing batching at present (the iterate takes approx. 0.2 sec on this large dataset and will be much smaller on a smaller dataset). Let me know if this works for you
There was a problem hiding this comment.
Test now takes 0.1 secs on the raw data from an rplidar (808 points)
There was a problem hiding this comment.
carto output: 180,000 points. + 15 secs
pointcloud/bun090.pcd: 30,000 points + 2.7 sec
rplidar_data: 800 points + 0.1 sec
@tessavitabile @nicksanford please confirm that we are alright with the 800 point pcd file instead of the 30,000 that takes 2.7 sec
pointcloud/basic_octree_test.go
Outdated
| @@ -557,8 +557,19 @@ func TestBasicOctreePointcloudIngestion(t *testing.T) { | |||
|
|
|||
| // Test the functionalities involved with converting a pcd into a basic octree. | |||
| func TestReadBasicOctreeFromPCD(t *testing.T) { | |||
| artifactPath := "pointcloud/test_short.pcd" | |||
| t.Run("reading from binary PCD to octree", func(t *testing.T) { | |||
| binaryArtifactPath := "slam/example_cartographer_outputs/viam-office-02-22-1/pointcloud/pointcloud_15.pcd" | |||
There was a problem hiding this comment.
Can you find out what is taking 15s now? I want to make sure that Iterate() isn't slower than expected.
| @@ -557,8 +557,19 @@ func TestBasicOctreePointcloudIngestion(t *testing.T) { | |||
|
|
|||
| // Test the functionalities involved with converting a pcd into a basic octree. | |||
| func TestReadBasicOctreeFromPCD(t *testing.T) { | |||
There was a problem hiding this comment.
This test seems to check that all points in the pointcloud are also in the octree. Do you know why it passed previously? Does it fail now if the points are different?
There was a problem hiding this comment.
See conversation here: https://viaminc.slack.com/archives/C036N5GJFLM/p1678115565635339
There was a problem hiding this comment.
Just to confirm, without the fix, the test does fail on large input sizes? And we're now testing large enough input sizes that we would catch the regression?
viambot
added
safe to test
| pcdFile, err := os.Open(artifact.MustPath(artifactPath)) | ||
|
|
||
| path := filepath.Clean(artifact.MustPath(artifactPath)) | ||
| pcdFile, err := os.Open(path) |
There was a problem hiding this comment.
Don't we need to defer close?
.artifact/tree.json
Outdated
| @@ -50623,6 +50623,7 @@ | |||
| "size": 145250924 | |||
| }, | |||
| "example_cartographer_outputs": { | |||
| "position": {}, | |||
There was a problem hiding this comment.
not sure why, and removed
viambot
added
safe to test
viambot
added
safe to test
viambot
added
safe to test
viambot
added
safe to test
viambot
added
safe to test
go.mod
Outdated
| github.com/pion/webrtc/v3 v3.1.54 | ||
| github.com/pixiv/go-libjpeg v0.0.0-20190822045933-3da21a74767d |
| @@ -557,8 +557,19 @@ func TestBasicOctreePointcloudIngestion(t *testing.T) { | |||
|
|
|||
| // Test the functionalities involved with converting a pcd into a basic octree. | |||
| func TestReadBasicOctreeFromPCD(t *testing.T) { | |||
There was a problem hiding this comment.
Just to confirm, without the fix, the test does fail on large input sizes? And we're now testing large enough input sizes that we would catch the regression?
There was a problem hiding this comment.
@tessavitabile This type of error can only occur on files with ~247 points. Both of our files have +800 so we should detect it if there is an issue
viambot
added
safe to test
viambot
added
safe to test
|
This PR fixes a bug with reading a PCD into a basic octree caused by the
bufio.Readerbeing unable to be copied/duplicated. This led to the initial read to get metadata for the basic octree creation, leaving no bytes left for the actual adding process.The fix has been to read the whole buffer to a string which then can be given to two separate
bufio.Readerobjects.Testing: Tested on multiple pcds of different sizes and types (ascii vs binary).
Open Question: In this PR the full PCD of the office is used for binary stress testing which takes approx 15 secs to be tested due to the presence of over 189,000 points. Do we want to use a small dataset for binary PCD testing or are we fine with the 15 sec time?
Additional Details:
asciiArtifactPath := "slam/rplidar_data/rplidar_data_0.pcd"
binaryArtifactPath := "slam/example_cartographer_outputs/viam-office-02-22-1/pointcloud/pointcloud_15.pcd"
JIRA Ticket: RSDK-1870