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.

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

Remove redundant Modulus operation for Voxel Hash function #358

Merged
merged 13 commits into from
Jul 9, 2024

Conversation

saurabh1002
Copy link
Contributor

@saurabh1002 saurabh1002 commented Jul 4, 2024

  1. Remove the modulus operation (1 << 20) - 1 & <hash> from the custom Hashing operator.

2. Initialize the robin map with a higher bucket size, change the default load factors. #359

Description by Gupta

The modulus operation in the hashing operator of the custom hash function is actually not required by the Robin Map implementation. They perform that operation internally based on the current bucket size, which keeps on changing based on current load_factor of the hash table. See the figure below where I backtrace to that modulus operation in Robin Map library.

In fact, the current bucket size that we have, through the hash operator, is 2^20 buckets. This can cause an issue, when internally the robin map grows to more than 2^20 buckets, in which case our hash function will limit the robin map bucket computation. Otherwise, when the internal bucket size is below 2^20, our additional modulus operation is just redundant.

@benemer
Copy link
Member

benemer commented Jul 4, 2024

Can you please give some pointers to the documentation where this can be found? Especially that the modulus operation is done on the hash map side

@saurabh1002
Copy link
Contributor Author

saurabh1002 commented Jul 4, 2024

The Hashing function

The modulus operation in the hashing operator of the custom hash function is actually not required by the Robin Map implementation. They perform that operation internally based on the current bucket size, which keeps on changing based on current load_factor of the hash table. See the figure below where I backtrace to that modulus operation in Robin Map library.

In fact, the current bucket size that we have, through the hash operator, is 2^20 buckets. This can cause an issue, when internally the robin map grows to more than 2^20 buckets, in which case our hash function will limit the robin map bucket computation. Otherwise, when the internal bucket size is below 2^20, our additional modulus operation is just redundant.

image
image

@saurabh1002
Copy link
Contributor Author

Default load factor params

The robin map internally resizes the hash table based on the load_factor thresholds.

  1. It upscales the bucket size by a factor of 2 when load_factor goes above 0.5, which is an expensive operation as it recomputes all the hashes and the buckets they are assigned to.
  2. It downscales the bucket size by a factor of 2 again, when the load_factor goes below 0.0, which means basically never.

It is better if we already initialize the robin map with a sufficient bucket size, along with increasing the maximum load factor threshold for upscaling to 0.75. This will avoid frequent rehashing of the map, especially at the beginning of any sequence.
Also, we can set the minimum load factor threshold for downscaling to 0.05 to have some sort of downscaling when operating in narrow regions with lower FOV, and avoid costly iterations through the map, for example when querying for nearest neighbors, or generating pointcloud from local map.

@saurabh1002
Copy link
Contributor Author

image

@saurabh1002
Copy link
Contributor Author

saurabh1002 commented Jul 4, 2024

Test on MulRAN Sequences

image
image

@saurabh1002
Copy link
Contributor Author

Apollo Dataset

image

@nachovizzo nachovizzo added the core label Jul 5, 2024
@nachovizzo nachovizzo self-requested a review July 9, 2024 12:58
Copy link
Collaborator

@nachovizzo nachovizzo left a comment

Choose a reason for hiding this comment

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

Fantastic @saurabh1002 thanks !

@nachovizzo nachovizzo merged commit e19823a into PRBonn:main Jul 9, 2024
17 checks passed
@nachovizzo nachovizzo added the voxelization All the topic related to voxelization utilities label Aug 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core voxelization All the topic related to voxelization utilities
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants