theochem / procrustes

Python library for finding the optimal transformation(s) that makes two matrices as close as possible to each other.
https://procrustes.qcdevs.org/
GNU General Public License v3.0
109 stars 20 forks source link

Testing for PSDP #173

Closed banrovegrie closed 2 years ago

banrovegrie commented 2 years ago

The first commit deals with #172. Scaling, translation etc. have been fixed for the input arrays. There are some changes to docstring too. Namely:

FanwangM commented 2 years ago

Is this PR complete? You can tag me wherever you think it's ready for code review. Thank you. @banrovegrie

banrovegrie commented 2 years ago

Yea, I will tag you as soon as this is complete.

banrovegrie commented 2 years ago

@FanwangM initially my tests were failing now I have fixed the basic tests. I will open a new PR for uploading the new algorithm's implementation

banrovegrie commented 2 years ago

This PR relates to the tests required for #98.

FanwangM commented 2 years ago

One more tip. For merging the upstream repo code to your local branch, a better way is to use git rebase. For example, if your current working branch is test-psdp, you can do

git checkout master
# suppose you have setup git@github.com:theochem/procrustes.git as your upper steam, named theochem
git pull theochem master
# if you want to update your master branch remotely
# git push origin master
git checkout test-psdp
git rebase master
git push test-psdp

A good point of doing this is that we will avoid the commit history of merging upstream codes. If you want to practice, you can close this PR and create a new one following this trick. But it doesn't matter too much whichever way you go. @banrovegrie

More details can be found at https://www.atlassian.com/git/tutorials/merging-vs-rebasing and https://git-scm.com/docs/git-rebase.

banrovegrie commented 2 years ago

Oh yeah, I should rebase instead of the way I merged. I will keep that in mind from now on. Thanks!

banrovegrie commented 2 years ago

@FanwangM after we merge this, I will open the PR for algorithm 2 (Peng et al).

Some issues with the Peng Algorithm:

  1. What does $\ast$ denote in the definition of $\hat{S}$? $$\hat{S} = \Phi \ast (U^T_1 F V_1 \Sigma + \Sigma V^T_1 F^TU_1)$$

  2. Doesn't $\Phi \in \mathbb{R}^{r \times r}$ rather than $\mathbb{R}^{n\times n}$?

  3. What does $\hat{P{11}^{+}}$ in the paper? Am I supposed to take transpose conjugate or something (which doesn't make sense)? Also, $\hat{P{11}}$ by default has all of its eigenvalues $\geq 0$.

banrovegrie commented 2 years ago

Thanks a lot!