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

Preprocessing Pipeline #412

Merged
merged 31 commits into from
Jan 8, 2025
Merged

Conversation

tizianoGuadagnino
Copy link
Collaborator

@tizianoGuadagnino tizianoGuadagnino commented Dec 5, 2024

Motivation

I finally wanted to tackle #295, as the preprocessing has been an asymmetry in the design for a while, and I would like to have an emotional closure with it. As a side effect, this also allows control of the number of threads for deskewing, as now all preprocessing is performed in a single module (similar to the attempt already made in #400).

This PR

Introduce the Preprocessor module, which is in charge of clipping the scan to the min/max range and performing the motion compensation. For now, I kept the voxel downsampling outside the preprocessing pipeline, although we could merge it in this module if we like the idea.

Results

Of course, I obtained the same numbers in terms of runtime and "accuracy" for the few sequences I tested.

@tizianoGuadagnino tizianoGuadagnino added enhancement New feature or request core labels Dec 5, 2024
@tizianoGuadagnino tizianoGuadagnino self-assigned this Dec 5, 2024
@tizianoGuadagnino tizianoGuadagnino linked an issue Dec 5, 2024 that may be closed by this pull request
3 tasks
@tizianoGuadagnino tizianoGuadagnino changed the base branch from main to tiziano/concurrent_vector December 5, 2024 16:35
@benemer
Copy link
Member

benemer commented Dec 18, 2024

Amazing! It looks good to me. Now we also use tbb for deskewing + only deskew the points that are not clipped :)

It's OK to keep the voxel downsampling for now because we also use it in the VoxelHashMap, which is not part of Preprocessing.

benemer
benemer previously approved these changes Dec 18, 2024
Base automatically changed from tiziano/concurrent_vector to main January 7, 2025 10:33
@tizianoGuadagnino tizianoGuadagnino dismissed benemer’s stale review January 7, 2025 10:33

The base branch was changed.

@tizianoGuadagnino
Copy link
Collaborator Author

Successive commits represents the fact that me and @benemer realize that the numbers were differing slightly from main in terms of accuracy, while the runtime seemed to be affected the most based on single thread vs multi-thread. For these reasons we converged to the current solution which replicates the code present in main but wrap it into a function so that we can get both a separate module for the preprocessing and control over the threads used for deskewing, while getting the same numbers for runtime and accuracy.

@tizianoGuadagnino tizianoGuadagnino merged commit 1642261 into main Jan 8, 2025
19 checks passed
@tizianoGuadagnino tizianoGuadagnino deleted the tiziano/preprocessing_pipeline branch January 8, 2025 12:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Modularize Preprocessing Pipeline
2 participants