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

[BUG] Missing transpose in AffineRegistration #75

Open
necoxt opened this issue Nov 10, 2022 · 4 comments
Open

[BUG] Missing transpose in AffineRegistration #75

necoxt opened this issue Nov 10, 2022 · 4 comments

Comments

@necoxt
Copy link

necoxt commented Nov 10, 2022

Describe the bug
Hi! Thanks for the awsome code, it is very efficient!

I noticed that when I use the class AffineRegistration on my dataset, the result is occasionally wrong. I think the issue comes from "update_variance" method, in the calculation of "trBYPYP", "np.trace(np.dot(np.dot(self.B, self.YPY), self.B))" should be "np.trace(np.dot(np.dot(self.B.T, self.YPY), self.B))", which also equals to "np.trace(np.dot(self.A, self.B))". This is verified in another project probreg.

To Reproduce
Here are my source and target points which cause the code to fail using AffineRegistration:
source.txt
target.txt

Expected behavior
AffineRegistration should work for the given dataset.

Screenshots
Current matching results:
Screen Shot 2022-11-10 at 3 33 40 PM
Expected matching results:
Screen Shot 2022-11-10 at 3 35 23 PM

@gattia
Copy link
Collaborator

gattia commented Nov 29, 2022

Hey, thanks for pointing this out - and for doing the debugging!

Just to make sure - did you try making this fix and then running the code to make sure it works?

@necoxt
Copy link
Author

necoxt commented Nov 30, 2022

Sure, the second picture above is the result after making the fix.

@gattia
Copy link
Collaborator

gattia commented Dec 7, 2022

@necoxt -Thanks so much for looking into this and reporting the potential bug here!

Have you tried running the code you referenced at: probreg on this problem? Did it produce the results you expect?

I say this because these two code bases arent identical, for example, it looks like in general when B is transposed in our library b is not transposed in probreg library. E.g., here vs. here, here vs. here, and here vs here.

I think the root cause to this difference in transpose is because when b is defined in probreg they transpose it immediately here whereas this repository does not transpose it here.

Having said that, there are a few other differences in the code implementation and I dont have the time right now to fully asses if any of those have an affect on this.

If you have other thoughts on why this should be transposed please let me know and I am happy to look more into it. Or if the other repository does work on your data, that would be really nice to know as well as it means that we should definitely be looking into fixing this.

Thank you!

@gattia
Copy link
Collaborator

gattia commented Nov 9, 2023

@necoxt - just checking if you ever tried this using the probreg? I just want to make sure it worked as expected before we dig into this more.

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

No branches or pull requests

2 participants