search

siavashk / pycpd

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

Adding correspondence priors to cpd #57

Closed agporto closed 1 year ago

agporto commented 2 years ago

In my pycpd fork, I've implemented the cpd extension described in https://ieeexplore.ieee.org/document/7477719 It allows users to add correspondence priors with varying degrees of confidence (from reliable to unreliable) to the usual cpd algorithm. It works both for the low rank as well as the standard cpd implementation. So, I thought I would send a pull request upstream in case there is broader interest in it. I've tested it in the context of biological research and it seems to be working well (no problems that I could observe). Let me know if you think it could be useful (or maybe it won't). In any case, here it goes. @gattia

siavashk commented 2 years ago

Hello @agporto!

Thank you for doing this. Can you also write an example that showcases the registration algorithm? I looked at the paper and it seems that they use this dataset: http://grail.cs.washington.edu/data/scans/

agporto commented 2 years ago

Hello @agporto!

Thank you for doing this. Can you also write an example that showcases the registration algorithm? I looked at the paper and it seems that they use this dataset: http://grail.cs.washington.edu/data/scans/

Will do! I will update the pull request sometime in the next week or so.

agporto commented 2 years ago

Hello @siavashk I've now updated the pull request with 3 examples (2D, 3D, lowrank) using the fish dataset that was already contained in the repo. In these examples, I simulate target pointclouds missing extensive parts. I checked out the dataset you suggested, but in the paper they use intrinsic shape descriptors to establish the initial correspondences. I did not want to add any extra dependency to pycpd, so the fish dataset seemed simpler. Please let me know what you think.

siavashk commented 2 years ago

This looks really good. I have to review it over the weekend but for now 👍

gattia commented 2 years ago

@agporto Thanks for this! And sorry I've been checked out on this one so far. I'm still going to quickly review over the weekend, but in the meantime, do you have an objection if I switch this to be a pull-request off of the development branch instead? Im hoping to get a batch of these pull requests through to the development branch and then we can merge them all at once.

Also, is there a test you can think of for this? If so, do you mind adding it into the tests?

Thank you!

agporto commented 2 years ago

@gattia No worries. I am happy to add some testing (probably this week). It will likely follow the pattern you have set. And yes, please feel free to switch the pull request to the development branch, but maybe you could wait until I update this pull request with testing (it is currently updating the pull request automatically from my branch). I will send you a message when I update the testing part.
Thanks!

gattia commented 2 years ago

Thanks! That all sounds perfect.

agporto commented 2 years ago

Hi @gattia. The McKinney fire caused me to delay it a bit, but here is the testing file. Please let me know if there is anything that is not clear. Please feel free to change it to the development branch at this point. Thanks!

gattia commented 1 year ago

@agporto - Im very sorry for the long delay on this. It's taken me a while to sit down and look it all over. This looks great. I've made a few tweaks and pushed them to your repository.

  1. removed some redundancy in creating the gaussian kernel etc. (https://github.com/siavashk/pycpd/pull/57/commits/305b6fdcd1c275dcaa00b0b808541835b9768658)
  2. updated the examples to include coloring that identifies what points are constraining the registration. (https://github.com/siavashk/pycpd/pull/57/commits/5a8015ae2b960c409494050c8beb269d7fafcdb5)

Let me know what you think about these. If you're OK with them, then I'll proceed to merging them with the development branch and then master.

agporto commented 1 year ago

@gattia Looks great. Makes sense to remove the redundancy. And the identification of the points constraining registration is very useful. So, please feel free to proceed with merging. There are a few conflicts with main branch to resolve. One is essentially due to the redundancy removal. The other is related to the "is not" syntax in the main repo (which I substituted in my repo for !=). I will leave those conflicts for you to resolve, if that is ok with you.

gattia commented 1 year ago

@agporto - that's fine, I'll move to development and then fix everything over there. Thanks for this contribution!

gattia commented 1 year ago

@agporto - I made one final set of changes where I had your new class inherit from the base deformable_registration class to reduce some redundancy and let the new class gain any bug fixes or other changes that the base class receives.

Thank you so much for this addition! It was clearly a lot of work and is an excellent contribution to the library.

Best,

Anthony

agporto commented 1 year ago

@gattia It was my pleasure. Thanks to you and @siavashk for pycpd in the first place. It has been very helpful in my research.