scikit-image

scikit-image copied to clipboard

Description

This is a tiny follow-up on https://github.com/scikit-image/scikit-image/pull/6840#issuecomment-1525688968.

Not sure about 5e8e1a84a7: 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...

Checklist

• A descriptive but concise pull request title
• Docstrings for all functions
• Unit tests
• A gallery example in ./doc/examples for new features
• Contribution guide is followed

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`.

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

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.

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

I have reverted 5e8e1a84a75ae6a8e4dd456dbe31ca4ee4884ea7 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.

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

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

sign is ignored

Thanks, I wouldn't have guessed.

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

Ok, done f662e90d986999f329141e451a77da0c836d0238.

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

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

"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.

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.