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

Refactor #55

Closed FarnazH closed 3 years ago

FarnazH commented 3 years ago

I have been refactoring (mostly) the one-sided Procrustes methods. I am still working on this, but to avoid huge PRs, I thought to share this with you for reviewing and merging. Please let me know if you have any questions.

PaulWAyers commented 3 years ago

@FarnazH had talked to me about this. Since Procrustes only works for matrices with the same shape (except for one exception that is Issue #49 and is not implemented yet), why confuse the user by letting them choose a pad option that will crash and burn? It seems better to default to padding "however is necessary so we can run" but allow the user to turn this off, so that if Procrustes is used inside a larger program, you can choose to fail if the input should be improper (to ensure, for example, that you don't inadvertantly pass the transpose of a matrix you are concerned with).

The flexible options of padding are needed in the utility module, however, because in some cases (2-sided orthogonal, until we extend that method that at least) you need a square matrix.

Does this make sense @fwmeng88 ? I couldn't think of any cases where the "reference" and "input" matrices were not required to have the same shape (except for the aforementioned issue).

FanwangM commented 3 years ago

This makes a lot of sense! I will fix a few minor coding style issues and merge this PR shortly.

Thanks for the clarifications! @PaulWAyers Thanks for refactoring the codes! @FarnazH