-
-
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
Update docstring for SimilarityTransform. #7194
Draft
mkcor
wants to merge
6
commits into
scikit-image:main
Choose a base branch
from
mkcor:update-transform-docstring
base: main
Could not load branches
Branch not found: {{ refName }}
Could not load tags
Nothing to show
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
68657c4
Update docstring for consistency
mkcor 62c19ca
Merge branch 'main' into update-transform-docstring
mkcor 0c49315
Use consistent notation for dimensionality
mkcor 12b6b83
Propagate update to EuclideanTransform docstring
mkcor 8901b8f
Propagate update to ProjectiveTransform docstring
mkcor e0baa7b
Propagate update to other docstrings
mkcor File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't mind updating this to "n" or "N". Though I do notice that translation is only supported for 2D and 3D. So the
...
should be removed as specifying more than xyz wouldn't work?params : (dim+1, dim+1)
(below) should also be updated according to whatever we agree on here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, thanks @lagru.
We can't use
N
because this already denotes something else (number of points):scikit-image/skimage/transform/_geometric.py
Line 42 in d6ba6db
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find
D
(Dimensionality) to be much clearer thann
, particularly when next toN
(Number of points)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@scott-vsi I hear you. Have you scanned the other modules to see which usage would currently dominate? Should we update "nD image" everywhere with "D-dim image?" In a sentence, "nD image" seems much more natural though...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, nD reads better, although it looks like n-D and N-D are also used pretty commonly (there are actually not that many uses of nD. I could see changing those to n-D or N-D).
skimage is a large package, so I doubt you could ever get complete consensus, but it looks like
D
dominates for the dimensionality of the image. It is used in skimage._shared.utils.check_nD, skimage.measure._moments.moments_coords, skimage.measure._regionprops_utils.euler_number, and skimage.feature.template.match_template. And that is just from a quick spot check.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for looking into this, @scott-vsi! I'll mark this PR as draft, because I realize some discussion is needed before we can move forward.
I would avoid uppercase N because, in addition to number of points #7194 (comment), this notation is also used for number of columns, e.g.:
scikit-image/skimage/transform/_geometric.py
Line 98 in d6ba6db