-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Consider renaming x/y/z to plane/row/col in the 3D structure tensor example #5275
Comments
The wording change is fine by me 😉. |
@jni in that example, I intended the x/y 'inversion,' as made explicit with code line labels={'x': 'Y', 'y': 'X', 'color': 'intensity'} but I'm definitely fine with replacing z/y/x by plane/row/col. I 'inherited' the dataset from #4946 where I had derived that Dimensions are provided in the following order: (z, y, x, c). n_plane, n_Y, n_X, n_chan = data.shape see rendered (not yet merged) tutorial here. Right now I can't remember what convinced me of the above -- @GenevieveBuckley would you have a clue by any chance? I agree it's confusing, because |
I don't know, sorry @mkcor |
@GenevieveBuckley no worries! @jni I don't remember what crossed my mind and got me into a self-convincing loop... Anyhow, sorry for the confusion, I fixed it with #5282 (and 9656e41). I agree it's important to keep the plane/row/col[/channel] convention, consistently with the tutorial introducing 3D image processing. @GenevieveBuckley would you like me to use the #5282 opportunity to remove the explicit mention of your authorship, as in your code review? Thanks! |
Sure, go ahead |
Description
I didn't find any discussion in #5012 about axis naming, but to me it seems a bit muddled... We normally would call the vertical axis Y and the horizonal axis X, but in the 3D structure tensor they are inverted.
Since we are striving to use planes/rows/columns throughout the library, I'm wondering @mkcor @grlee77 @rfezzani whether you would support changing the wording in that example to use that nomenclature. Happy to provide the PR for this if there is consensus.
The text was updated successfully, but these errors were encountered: