Refactor #55
Refactor #55
Conversation
The order of arguments were changed to make it consistent with setup_input_arrays and setup_input_arrays_multi.
The notation is now consistent with the paper.
1. Change pad_mode argument to pad 2. Change the order of arguments 3. Set default values of remove_zero_col & remove_zero_row to False
1. Change variable names to a & b 2. Change the order of arguments 3. Change pad_mode to pad 4. Set default values of remove_zero_col & remove_zero_row to False 5. Update documentation & comments
1. Change variable names to a & b 2. Change the order of arguments 3. Change pad_mode to pad 4. Set default values of remove_zero_col & remove_zero_row to False 5. Update documentation & comments
Random matrices are used to test generic procrustes. This led to realizing that the transformation matrix is not unique when there are fewer rows than columns, so the docstring was updated to mention that.
There was a problem hiding this comment.
The codes look great to me. Thanks for all the work you have done! It's a very impressive improvement for our package.
My only hesitation is about the argument pad in all the single-sided permutation procrustes methods. The original pad_mode argument provides four different options for people to perform zero-padding. Can you clarify this a little more? Thanks!
|
@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). |
|
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 |
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.