Faster lookup #190
Faster lookup #190
Conversation
Currently all our tests are failing because the garden tagged images were removed. This retargets the tests to use the latest tag. Signed-off-by: Arjo Chakravarty <[email protected]>
As pointed out by @hidmic , #174 makes test_range_bearing redundant. This PR eleminates the test and the associated files. Signed-off-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Arjo Chakravarty <[email protected]>
|
|
||
| // Spawn first vehicle | ||
| ignition::transport::Node node; | ||
| node.Subscribe("/model/vehicle1/chlorophyll", &ChlorophyllCb); |
There was a problem hiding this comment.
@arjo129 meta: much of this test is preparation in order to check for science data. Why not crafting a world SDF that spawns the vehicle at the desired location?
There was a problem hiding this comment.
Actually, I eventually want to eliminate the zoo of worlds we have and focus just on empty_environment.sdf. Ideally we would write spawning infra to make it easy. For now I have not done that yet.
There was a problem hiding this comment.
@arjo129 I agree with not having lots of worlds that are only slightly different. At the same time, I'm not sure if we can (should?) accommodate every test in the same world e.g. for some tests we may have to ensure there are no external forces acting on the vehicle, while others require them. Crafting the world in runtime also adds unrelated failure modes to a test, like in this case, where 90%+ of the assertions are accessory to what is being tested.
In my mind, having a generic base SDF and extending it with test-specific SDF snippets would be ideal. Unclear if that's possible though.
| @@ -319,20 +170,14 @@ class tethys::ScienceSensorsSystemPrivate | |||
| /// this->timestamps. | |||
| /// Point cloud: spatial coordinates to index science data by location | |||
| /// in the ENU world frame. | |||
| /// TODO(arjo): remove dependence on PCL. We literally are only using it for | |||
| /// the visuallization. | |||
There was a problem hiding this comment.
@arjo129 typo:
| /// the visuallization. | |
| /// the visualization. |
| public: std::vector<pcl::octree::OctreePointCloudSearch<pcl::PointXYZ>::Ptr> | ||
| spatialOctrees; | ||
| /// \brief Spatial index of data. | ||
| /// Vector size: number of time slices. Indices correspond to those of |
There was a problem hiding this comment.
@arjo129 nit:
| /// Vector size: number of time slices. Indices correspond to those of | |
| /// Vector size: number of time slices. Indices correspond to those of this->timestamps. |
?
| points.push_back( | ||
| ignition::math::Vector3d(this->timeSpaceCoords[i]->at(j).x, | ||
| this->timeSpaceCoords[i]->at(j).y, | ||
| this->timeSpaceCoords[i]->at(j).z)); |
There was a problem hiding this comment.
@arjo129 nit: index this->timeSpaceCoords once in the outer loop, as in:
const auto & timeSpaceCoord = this->timeSpaceCoords[i];Signed-off-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Arjo Chakravarty <[email protected]>
|
I'll re-review when we merge #191 in. |
|
Superceded by #191 |
This PR depends on:
It supersedes #188 and only focuses on the lookup thus making the reviewers life easier.
It implements a fast lookup for grid data like the data provided by MBARI. It also adds an integration test for the science data. Currently we are only checking data that does not vary with time. I have removed all interpolation code from this PR to keep it clean. I will be adding it back in the next PR. On my Laptop's Ryzen 9 5900HX this code takes between 3-15 microseconds per lookup.