8000 rename img_as_* to rescale_to_* by grlee77 · Pull Request #6318 · scikit-image/scikit-image · GitHub
[go: up one dir, main page]
Skip to content

rename img_as_* to rescale_to_* #6318

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.

Sign up for GitHub

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

Jump to bottom
Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

rename img_as_* to rescale_to_* #6318

wants to merge 5 commits into from

Conversation

grlee77
Copy link
Contributor
@grlee77 grlee77 commented Apr 5, 2022

Description

This PR implements a prior suggestion to rename img_as_float to rescale_to_float, etc. This is done primarily to make it clearer that these function rescale the range of the inputs. This change has been discussed a few of times in the past (#1234, #3009 (comment) and #5439)

At the moment, img_as_float, img_as_float32 and img_as_float64 are also still here, but no longer rescale the range. We can likely just go ahead and remove this as astype can already be used to convert dtypes without scaling.

Checklist

  • Docstrings for all functions
  • Gallery example in ./doc/examples (new features only)
  • Benchmark in ./benchmarks, if your changes aren't covered by an
    existing benchmark
  • Unit tests
  • Clean style in the spirit of PEP8
  • Descriptive commit messages (see below)

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.
  • There is a bot to help automate backporting a PR to an older branch. For
    example, to backport to v0.19.x after merging, add the following in a PR
    comment: @meeseeksdev backport to v0.19.x
  • To run benchmarks on a PR, add the run-benchmark label. To rerun, the label
    can be removed and then added again. The benchmark output can be checked in
    the "Actions" tab.

@grlee77
rename img_as_* -> rescale_to_*
d7dd70d 
update docs and gallery to use rescale_to_*
@grlee77 grlee77 added the 📜 type: API Involves API change(s) label Apr 5, 2022
@grlee77 grlee77 added this to the skimage2 milestone Apr 5, 2022
@pep8speaks
Copy link
pep8speaks commented Apr 5, 2022 •
edited
Loading

Hello @grlee77! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 198:1: E402 module level import not at top of file

Line 99:1: E402 module level import not at top of file

Comment last updated at 2022-04-05 17:52:16 UTC

This was referenced Apr 5, 2022
Open
Draft
lagru
lagru reviewed Apr 10, 2022
View reviewed changes
Copy link
Member
@lagru lagru left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks @grlee77. I'm not through yet but I have a few comments.

Also there still is a lonely img_as_float in doc/CONTRIBUTING.rst under "Stylistic Guidelines".

skimage/util/dtype.py Outdated
return _img_as_float(image, force_copy, preserve_range=False)


def _img_as_float(image, force_copy=False, *, preserve_range=True, dtype=None):
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 _rescale_to_float would be the clearer and more consistent name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

_img_as_float has been removed and _convert is just used directy now.

skimage/util/dtype.py Outdated
Comment on lines 468 to 470
return _convert(image, np.floating, force_copy)
return _img_as_float(image, force_copy, preserve_range=False)
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand this change. Why did you wrap _convert with the new function _img_as_float?

Also one could argue that it would be more explicit to use dtype=np.floating here and not rely on _img_as_float's implicit behavior for dtype=None. Or rather use np.floating as the default in the function signature?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure why I did that, will take a look again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

_img_as_float has been removed and _convert is just used directy now.

@grlee77
Copy link
Contributor Author
grlee77 commented Apr 15, 2022

Per #1234 (comment), we need to decide on rescale_to_* vs. convert_to_*. I prefer rescale_to because it is clearer that it rescales the range of the input and is not just a dtype conversion.

Also, we do already have a private convert_to_float utility that is used in a few places internally, although that one has a preserve_range kwarg.

@grlee77
Copy link
Contributor Author
grlee77 commented Apr 15, 2022

We will need to hold off on merging this PR until after we branch 1.0

@lagru lagru marked this pull request as draft April 16, 2022 16:02
@github-actions

This comment was marked as outdated.

Sign in to view
@github-actions github-actions bot added the 😴 Dormant No recent activity label Dec 12, 2022
@lagru
Copy link
Member
lagru commented Aug 8, 2024 •
edited
Loading

We will need to hold off on merging this PR until after we branch 1.0

Why? I'm assuming that 1.0 was the former "breaking API" milestone. This seems like a deprecation that doesn't require a major version or namespace change.

If this was decided on I would like to pick it up again. I'm also +1 on rescale_to over convert_to.

@lagru lagru removed the 😴 Dormant No recent activity label Aug 8, 2024
@stefanv stefanv added this to skimage2 Mar 1, 2025
@stefanv stefanv moved this to To Do in skimage2 Mar 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lagru lagru lagru left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
✋ Wait for skimage2 📜 type: API Involves API change(s)
Projects
skimage2
Status: To Do
Milestone
2.0
Development

Successfully merging this pull request may close these issues.
4 participants
@grlee77 @pep8speaks @lagru @alexdesiqueira
0