search

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

Peng Algorithm #180

Closed banrovegrie closed 2 years ago

banrovegrie commented 2 years ago

This is the PR involving the addition of the second algorithm to the PSDP module. Here, I have included a rough sketch of the algorithm.

The issues that remain have been described in discussion #179.

banrovegrie commented 2 years ago

Apparently, autopep8 converts some of the lambda functions into proper functions. I hope that is fine?

@FanwangM

banrovegrie commented 2 years ago

Tests won't pass for this commit since I have many linting issues which we can ignore for now.

FanwangM commented 2 years ago

This is another improvement. Thanks.

For the commit, ff04043, is is a similar operation that we have encountered in the previous PR? If you want to update the git history, we should use git rebase to have a clean history this time. If this is not what you mean for that commit, can you explain a little? Thank you.

banrovegrie commented 2 years ago

@FanwangM yeah, so the issue was that I had the commits added to the branch before test-psdp was added and somehow in the end I ended messing up while rebasing when i encountered a few merge conflicts.

banrovegrie commented 2 years ago

I think the code is ready to be reviewed btw. Once we merge this commit, I will proceed with updating the tests to include testing for this algorithm as well.

codecov[bot] commented 2 years ago

Codecov Report

Merging #180 (171a794) into master (ec238c4) will decrease coverage by 2.22%. The diff coverage is 30.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #180      +/-   ##
==========================================
- Coverage   84.42%   82.19%   -2.23%     
==========================================
  Files          13       13              
  Lines         751      775      +24     
==========================================
+ Hits          634      637       +3     
- Misses        117      138      +21
Impacted Files Coverage Δ
procrustes/psdp.py 74.44% <30.00%> (-22.53%) :arrow_down:
FanwangM commented 2 years ago

I noticed that you have two git merge operations. Can you try to use git rebase to avoid them. This can help us get a clean history.? You can either do (1) close this PR and open a new one; or (2) use more complicated git history editing operations to avoid these two. I would prefer the first one. Thanks. @banrovegrie

banrovegrie commented 2 years ago

Yeah, I will close this PR and open a new one.

banrovegrie commented 2 years ago

Closing this and pivoting to #187.