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

Optimized transformations by transposing and then multiplying in place #424

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

YoshuaNava
Copy link
Contributor

@YoshuaNava YoshuaNava commented Dec 2, 2020

After merging #419 I put some extra time in finding out if we could further optimize the transformation of descriptors.

Based on that I implemented some snippets:

I found out that for matrices like the ones we are processing (MxN, with M sort of small [1,10], but N quite large), it's better to transpose first, and then apply on the left. The Eigen parser seems to faster code when we do this.

image

Compared to what we had before:

image

The proportion of time spent between the operations is still similar 30-70% current, but the absolute time taken by the functions to process the data is shorter.


My thought on why this happens is that when we transpose we might be loading the matrix in L2/L3 cache (even though we don't enforce Eigen) in-time evaluation, and when the compiler sees applyOnTheRight, it optimizes for an in-place operation on a matrix that is dominantly column-based.

Something curious I found is that with compiler explorer you can try out different compilers, and the code runs just a bit faster with icc.

@pomerlef
Copy link
Collaborator

pomerlef commented Dec 2, 2020

image

@YoshuaNava
Copy link
Contributor Author

YoshuaNava commented Dec 2, 2020

@pomerlef I'm trying to reproduce the issue locally and on internal CI but I haven't been able to. On which platform are the jenkins tests failing?

@pomerlef
Copy link
Collaborator

pomerlef commented Dec 2, 2020

all

@pomerlef
Copy link
Collaborator

pomerlef commented Dec 2, 2020

It's seems to be related to 2D transformations. There are all binary test:

Error Message
/home/jenkins/workspace/pointmatcher/label/ubuntu-xenial/utest/ui/Transformations.cpp:39
Value of: transformedFeature.isApprox(transformedCloud.features.col(i), kEpsilonNumericalError)
  Actual: false
Expected: true
Stacktrace
/home/jenkins/workspace/pointmatcher/label/ubuntu-xenial/utest/ui/Transformations.cpp:39
Value of: transformedFeature.isApprox(transformedCloud.features.col(i), kEpsilonNumericalError)
  Actual: false
Expected: true

@YoshuaNava
Copy link
Contributor Author

YoshuaNava commented Dec 3, 2020

I quickly looked into the issue, and the problem appeared because the results are slightly different with the new method for transforming features. I'm not sure whether this is because:

This issue only pops up on my PC, with a fork of libpointmatcher, when I set the epsilon for error checking to 1e-13. On libpointmatcher from this repo (upstream), it happens with 1e-8.

I'm looking into this.

@pomerlef
Copy link
Collaborator

pomerlef commented Jul 6, 2021

@YoshuaNava could you have a look to resolve conflicts?

@pomerlef
Copy link
Collaborator

@YoshuaNava could you verify the conflict?

@boxanm boxanm changed the base branch from master to develop November 4, 2023 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants