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

Add kwarg source_range to cumulative_distribution and equalize_hist #4056

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

sciunto
Copy link
Member

@sciunto sciunto commented Jul 25, 2019

Description

Xref #4054

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.
  • Consider backporting the PR with @meeseeksdev backport to v0.14.x

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.

@sciunto I've requested a couple of very minor changes. Approved pending those.

skimage/exposure/exposure.py Outdated Show resolved Hide resolved
skimage/exposure/exposure.py Outdated Show resolved Hide resolved
sciunto and others added 2 commits July 26, 2019 14:30
Co-Authored-By: Juan Nunez-Iglesias <juan.nunez-iglesias@monash.edu>
Co-Authored-By: Juan Nunez-Iglesias <juan.nunez-iglesias@monash.edu>
@sciunto
Copy link
Member Author

sciunto commented Jul 26, 2019

Absolutely, it's done.

Number of bins used to calculate histogram. This value is ignored for
integer arrays, for which each integer is its own bin.
source_range : string, optional
'image' (default) determines the range from the input image.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
'image' (default) determines the range from the input image.
'image' (default) determines the range from minimum and maximum value of the input image.

is this true?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it's true, but not PEP8 compliant. =P

Copy link
Member Author

@sciunto sciunto Jul 31, 2019

Choose a reason for hiding this comment

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

I'm advocating to have this option set to 'source' by default. Although 'image' can be useful, it is not the most natural option to me. Especially, once you want to compare histograms, different ranges do not help. But, this is another story.

Copy link
Member Author

Choose a reason for hiding this comment

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

@hmaarrfk Added and modified elsewhere also.

@jni
Copy link
Member

jni commented Aug 4, 2019

It doesn't seem related to this PR, but the travis failures look pretty serious! =\

@hmaarrfk
Copy link
Member

hmaarrfk commented Aug 4, 2019

I think those failures are because a bad version of qt is being pulled in. I really hate QT testing. Just look at how long Vispy has been trying to release their package, they are constantly stuck chasing changes in QT bugs

@hmaarrfk
Copy link
Member

hmaarrfk commented Aug 4, 2019

Doesn't this need an easy test?

@sciunto sciunto added this to the 0.17 milestone Aug 18, 2019
Copy link
Member

@hmaarrfk hmaarrfk left a comment

Choose a reason for hiding this comment

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

no test has been added for this new functionality.

Base automatically changed from master to main February 18, 2021 18:23
@jarrodmillman jarrodmillman modified the milestones: 0.17, 0.20 Jun 4, 2022
@lagru lagru removed this from the 0.20 milestone Oct 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants