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

Add missing minus in SimilarityTransform docstring #6840

Merged
merged 1 commit into from Aug 15, 2023

Conversation

akonsk
Copy link
Contributor

@akonsk akonsk commented Mar 16, 2023

added a missing minus in the homogeneous transformation matrix

Description

Checklist

  • Docstrings for all functions
  • Gallery example in ./doc/examples (new features only)
  • Benchmark in ./benchmarks, if your changes aren't covered by an
    existing benchmark
  • Unit tests
  • Clean style in the spirit of PEP8
  • Descriptive commit messages (see below)

For reviewers

  • Check that the PR title is short, concise, and will make sense 1 year
    later.
  • Check that new functions are imported in corresponding __init__.py.
  • Check that new features, API changes, and deprecations are mentioned in
    doc/release/release_dev.rst.
  • There is a bot to help automate backporting a PR to an older branch. For
    example, to backport to v0.19.x after merging, add the following in a PR
    comment: @meeseeksdev backport to v0.19.x
  • To run benchmarks on a PR, add the run-benchmark label. To rerun, the label
    can be removed and then added again. The benchmark output can be checked in
    the "Actions" tab.

added a missing minus in the homogeneous transformation matrix
@@ -1293,7 +1293,7 @@ class SimilarityTransform(EuclideanTransform):

where ``s`` is a scale factor and the homogeneous transformation matrix is::

[[a0 b0 a1]
[[a0 -b0 a1]
Copy link
Contributor Author

@akonsk akonsk Mar 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in my understanding, the equations are correct and the matrix is not.
the homogenous matrix should be:
[[s*cos_theta, -s*sin_theta, t_x],
[s*sin_theta, s*cos_theta, t_y],
[0,0,1]]
here:
a0 = s*cos_theta
b0 = s*sin_theta
a1 = t_x
b1 = t_y

in the correction you proposed there is a contradiction, as b0=s*sin_theta=-s*sin_theta

@rum1887
Copy link
Contributor

rum1887 commented Mar 16, 2023

Nice catch @akonsk !

@rum1887
Copy link
Contributor

rum1887 commented Mar 21, 2023

hi @akonsk , are you an Outreachy applicant?

@lagru lagru added the 📄 type: Documentation Updates, fixes and additions to documentation label Mar 25, 2023
@@ -1293,7 +1293,7 @@ class SimilarityTransform(EuclideanTransform):

where ``s`` is a scale factor and the homogeneous transformation matrix is::

[[a0 b0 a1]
[[a0 -b0 a1]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the edit, @akonsk! All good now, I recognize the notation found, for example, at: https://en.wikipedia.org/wiki/Transformation_matrix#Affine_transformations

@mkcor
Copy link
Member

mkcor commented Apr 27, 2023

@akonsk would you mind including the same docstring fix for class EuclideanTransform (line 1257)? Thanks!

Comment on lines 1297 to 1298
[b0 a0 b1]
[0 0 1]]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
[b0 a0 b1]
[0 0 1]]
[b0 a0 b1]
[0 0 1]]

(to preserve alignment)

@lagru lagru changed the title fixed SimilarityTransform documentation Add missing minus in SimilarityTransform docstring Aug 15, 2023
@lagru lagru merged commit b6be0ce into scikit-image:main Aug 15, 2023
21 checks passed
@stefanv stefanv added this to the 0.22 milestone Aug 15, 2023
@lagru
Copy link
Member

lagru commented Aug 15, 2023

Thanks @akonsk!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📄 type: Documentation Updates, fixes and additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants