Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes for calc_shape_dist() #38

Merged
merged 3 commits into from
Feb 8, 2024
Merged

Fixes for calc_shape_dist() #38

merged 3 commits into from
Feb 8, 2024

Conversation

astamm
Copy link
Contributor

@astamm astamm commented Feb 8, 2024

I found a couple of issues in find_rotation_seed_coord() that had repercussions on calc_shape_dist(). The latter now performs the same calculations as in curve_pair_align(). In particular, the object beta2best output by find_rotation_seed_coord() is properly rotated and scaled when rotation == TRUE and scale == TRUE which makes it directly useable in calc_shape_dist() and curve_pair_align() without the need to perform the rotation, scaling and group action again a posteriori.

calc_shape_dist() is also better documented and its output has been augmented.

  • I have not renamed already existing output components in order not to break existing code that uses the function, although I think that d and dx for amplitude and phase distances could have better names.
  • I added betascale which is intended to be the optimal scaling factor that has been applied to the second curve when scale == TRUE; it is nothing but the length of beta1 divided by the length of beta2;
  • we could output qscale as well but it is not currently the case.

Lastly, I feel that we should document that the output beta1 is slightly different from the input of the same name. by clarifying what the centring performed via calculatecentroid() does.

…_coord() instead of duplicated the rotation and curve_to_q() operations.
… for calc_shape_dist(). It should now do the same thing as in curve_pair_align().
@jdtuck jdtuck merged commit e772d66 into jdtuck:master Feb 8, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants