search

siavashk / pycpd

Pure Numpy Implementation of the Coherent Point Drift Algorithm
MIT License
510 stars 115 forks source link

[BUG] Missing transpose in AffineRegistration #75

Open necoxt opened 1 year ago

necoxt commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

gattia commented 1 year ago

@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 commented 9 months ago

@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.