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.

Sign up for GitHub

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

RSDK-1870 Data Loss in ReadPCDtoBasicOctree #1973

Merged
merged 11 commits into from
Mar 6, 2023
Merged

RSDK-1870 Data Loss in ReadPCDtoBasicOctree #1973

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
f69750f
duplicate bufio.Reader and add PCD tests of both ascii and bnary
jeremyrhyde Mar 2, 2023
364a757
remvoe space
jeremyrhyde Mar 2, 2023
2dbc6b0
move to smaller pcd
jeremyrhyde Mar 3, 2023
fe4864e
pr fixes
jeremyrhyde Mar 3, 2023
a2706d5
reset instead of =
jeremyrhyde Mar 6, 2023
468efac
reset instead of =
jeremyrhyde Mar 6, 2023
b4a55f4
revert
jeremyrhyde Mar 6, 2023
06860d6
Merge branch 'main' of github.com:viamrobotics/rdk into RSDK-1870_Oct…
jeremyrhyde Mar 6, 2023
f1eb455
make build lint
jeremyrhyde Mar 6, 2023
d3afe1a
Merge branch 'main' of github.com:viamrobotics/rdk into RSDK-1870_Oct…
jeremyrhyde Mar 6, 2023
aee56ce
make build lint
jeremyrhyde Mar 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion pointcloud/basic_octree_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See conversation here: https://viaminc.slack.com/archives/C036N5GJFLM/p1678115565635339

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

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"
Copy link
Member

Choose a reason for hiding this comment

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

  1. Thanks for calling out that this takes 15 seconds.
  2. Why does this take 15 seconds?
  3. Is it 15 seconds just to get an octree or is it the iterate function that is slow?
  4. If its the iterate function that is slow, can this be parallelized using the Iterate method parameters to complete within a second?
  5. Is 15 seconds for doing what this is doing expected behavior & acceptable to consider Map Representation complerte?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the additional details section, I note that the actual creation is only 0.3secs. I will look into on parallelizing the iterate calls

Copy link
Contributor Author

@jeremyrhyde jeremyrhyde Mar 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Note: the time for octree/pointcloud creations includes adding all the points

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you find out what is taking 15s now? I want to make sure that Iterate() isn't slower than expected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Test now takes 0.1 secs on the raw data from an rplidar (808 points)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

yes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

testPCDToBasicOctree(t, binaryArtifactPath)
})

t.Run("reading from ascii PCD to octree", func(t *testing.T) {
asciiArtifactPath := "slam/mock_lidar/0.pcd"
testPCDToBasicOctree(t, asciiArtifactPath)
})
}

// Helper function for testing basic octree creation from a given artifact.
func testPCDToBasicOctree(t *testing.T, artifactPath string) {
basicPC, err := makeFullPointCloudFromArtifact(t, artifactPath, BasicType)
test.That(t, err, test.ShouldBeNil)
basic, ok := basicPC.(*basicPointCloud)
Expand Down
8 changes: 5 additions & 3 deletions pointcloud/basic_octree_utils_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,9 +247,11 @@ func validateMetadata(t *testing.T, bOct *BasicOctree) {
test.That(t, bOct.meta.MinY, test.ShouldEqual, metadata.MinY)
test.That(t, bOct.meta.MaxZ, test.ShouldEqual, metadata.MaxZ)
test.That(t, bOct.meta.MinZ, test.ShouldEqual, metadata.MinZ)
test.That(t, bOct.meta.TotalX(), test.ShouldAlmostEqual, metadata.TotalX())
test.That(t, bOct.meta.TotalY(), test.ShouldAlmostEqual, metadata.TotalY())
test.That(t, bOct.meta.TotalZ(), test.ShouldAlmostEqual, metadata.TotalZ())

tol := 0.0001
Copy link
Member

Choose a reason for hiding this comment

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

What is tol?

Copy link
Contributor Author

@jeremyrhyde jeremyrhyde Mar 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

tolerance value, ill update the variable to make that more explicit

Copy link
Member

Choose a reason for hiding this comment

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

Could you please also add a comment why this is needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

test.That(t, bOct.meta.TotalX(), test.ShouldBeBetween, metadata.TotalX()-tol, metadata.TotalX()+tol)
test.That(t, bOct.meta.TotalY(), test.ShouldBeBetween, metadata.TotalY()-tol, metadata.TotalY()+tol)
test.That(t, bOct.meta.TotalZ(), test.ShouldBeBetween, metadata.TotalZ()-tol, metadata.TotalZ()+tol)
}

// Helper function to create lopsided octree for testing of recursion depth limit.
Expand Down
10 changes: 9 additions & 1 deletion pointcloud/pointcloud_file.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -545,7 +545,15 @@ func readPCDHelper(inRaw io.Reader, pctype PCType) (PointCloud, error) {
case KDTreeType:
pc = NewKDTreeWithPrealloc(int(header.points))
case BasicOctreeType:
meta, err := getPCDMetaData(*in, header)

// Extract data from bufio.Reader to make a copy for metadata acquisition
buf, err := io.ReadAll(in)
if err != nil {
return nil, err
}
in = bufio.NewReader(bytes.NewReader(buf))

meta, err := getPCDMetaData(*bufio.NewReader(bytes.NewReader(buf)), header)
if err != nil {
return nil, err
}
Expand Down