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

Stick with usual convention for 3D spatial coords #5282

Merged
merged 4 commits into from
Mar 25, 2021

Conversation

mkcor
Copy link
Member

@mkcor mkcor commented Mar 20, 2021

Description

Fixes #5275

Checklist

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.

@@ -53,7 +53,7 @@
data[n_plane // 2, :, :, 1],
zmin=v_min,
zmax=v_max,
labels={'x': 'Y', 'y': 'X', 'color': 'intensity'}
labels={'x': 'row', 'y': 'col', 'color': 'intensity'}
Copy link
Member

Choose a reason for hiding this comment

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

it's x=col, y=row. =)

Copy link
Member Author

Choose a reason for hiding this comment

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

Aww there we go!! It's because x=col and y=row that I loaded the dataset with the following dimension order: [z, y, x, c] = [plane, row, column, channel] 😌

Ok, so I'll revert 58eec1c (where I introduced the inversion) and proceed with the expected renaming 😅

Comment on lines 82 to 83
# (n_Z, n_X, n_Y) = sample.shape
# Z, X, Y = np.mgrid[:n_Z, :n_X, :n_Y]
Copy link
Member

Choose a reason for hiding this comment

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

I think this was ok before?

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, reverted with 0da52ca.

@@ -105,15 +105,16 @@
sample[0, :, :],
zmin=v_min,
zmax=v_max,
labels={'x': 'Y', 'y': 'X', 'color': 'intensity'},
labels={'x': 'row', 'y': 'col', 'color': 'intensity'},
Copy link
Member

Choose a reason for hiding this comment

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

As above: x=col, y=row

Copy link
Member

@jni jni left a comment

Choose a reason for hiding this comment

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

@mkcor thank you for the fix! 😊 I think some rows/cols got confused (x=cols and y=rows) but this is almost there!

@mkcor
Copy link
Member Author

mkcor commented Mar 25, 2021

@jni thanks for reviewing!

@jni
Copy link
Member

jni commented Mar 25, 2021

Thank you @mkcor! Since this is a doc fix I'm gonna go ahead and merge. If anyone finds further issues we can always update again. 😊

@jni jni merged commit 3c10558 into scikit-image:main Mar 25, 2021
@mkcor mkcor deleted the fix-3d-spatial branch March 31, 2021 12:22
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.

Consider renaming x/y/z to plane/row/col in the 3D structure tensor example
2 participants