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

Fix docstring (missing minus sign) for EuclideanTransform. #7097

Merged
merged 9 commits into from
Apr 9, 2024

Conversation

mkcor
Copy link
Member

@mkcor mkcor commented Aug 16, 2023

Description

This is a tiny follow-up on #6840 (comment).

Not sure about 5e8e1a8: I have right-aligned the matrix coefficients because I find it more readable, but I noticed that, in the docstring for ProjectiveTransform, they seem centred rather than right-aligned or left-aligned...

Closes #7380.

Checklist

Release note

Summarize the introduced changes in the code block below in one or a few sentences. The
summary will be included in the next release notes automatically:

Add missing minus sign in docstring of `skimage.transform.EuclideanTransform`.

@mkcor mkcor added the 📄 type: Documentation Updates, fixes and additions to documentation label Aug 16, 2023
@mkcor mkcor requested a review from lagru December 19, 2023 14:50
Copy link
Member

@lagru lagru left a comment

Choose a reason for hiding this comment

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

Looks good. Happy with either way to format whitespace. :) Thanks Marianne.

@stefanv
Copy link
Member

stefanv commented Dec 19, 2023

I think the 1 was wrong, and everything should be left-aligned, other than for signs.

@lagru
Copy link
Member

lagru commented Dec 19, 2023

The 1 as in the last element in the third row? If you look at the equation for rotation around the z-axis on Wikipedia, it seems to be correct.

image

@mkcor
Copy link
Member Author

mkcor commented Dec 19, 2023

The 1 is consistent with the plain equations (also in other docstrings for transforms of the same family).

I have reverted 5e8e1a8 where I had right-aligned two matrices. I'm not sure how I should left-align

        [[a0  -b0  a1]
         [b0  a0  b1]
         [0   0    1]]

though. Either

        [[a0  -b0  a1]
         [b0  a0   b1]
         [0   0    1]]

where the matrix elements are left-aligned, period, or

        [[a0  -b0  a1]
         [b0  a0   b1]
         [0   0    1 ]]

to preserve the array structure? Thanks.

@lagru
Copy link
Member

lagru commented Dec 19, 2023

I think Stéfan was suggesting something like this 🤷 (aligned on letter / number, sign is ignored):

        [[a0 -b0  a1]
         [b0  a0  b1]
         [0   0   1 ]]

@mkcor
Copy link
Member Author

mkcor commented Dec 20, 2023

sign is ignored

Thanks, I wouldn't have guessed.

        [[a0 -b0  a1]
         [b0  a0  b1]
         [0   0   1 ]]

Ok, done f662e90.

@mkcor
Copy link
Member Author

mkcor commented Apr 4, 2024

@lagru should I label this PR 'quick win' or milestone 0.23?

@imagesc-bot
Copy link

This pull request has been mentioned on Image.sc Forum. There might be relevant details there:

https://forum.image.sc/t/is-this-a-tiny-bug-in-source-code-comment-and-document-eclidian-transform-in-skimage-trasform-geometric/94605/4

@lagru
Copy link
Member

lagru commented Apr 9, 2024

"Quick win" would have been appropriate I think. But I'll just merge this now, as it has been open for some time with no open concerns. It's also minor and easy to revert.

@lagru lagru merged commit b8ef57a into scikit-image:main Apr 9, 2024
22 checks passed
@stefanv stefanv added this to the 0.23 milestone Apr 9, 2024
@lagru
Copy link
Member

lagru commented Apr 9, 2024

This way it also goes into v0.23. @jarrodmillman, let me know if I should have postponed this until after the release and I'll do so in the future with similar cases.

@mkcor mkcor deleted the fix-minus-sign branch April 9, 2024 14:40
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.

A typo in source code comment and document
4 participants