Skip to content

Make the filternorm prop of Images a boolean rather than a {0,1} scalar. #10442

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
Merged
merged 1 commit into from
Apr 11, 2018
Merged

Make the filternorm prop of Images a boolean rather than a {0,1} scalar. #10442

merged 1 commit into from
Apr 11, 2018

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Feb 13, 2018

This seems a bit more pythonic and the C-API handles bools just as well.
Also filternorm and filterrad are always defined (as bool and float,
respectively, so no need to add ... or 0.0 when passing them to the
C-API.

PR Summary

PR Checklist

  • Has Pytest style unit tests
  • Code is PEP 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@anntzer anntzer added this to the v3.0 milestone Feb 13, 2018
@QuLogic
Copy link
Member

QuLogic commented Feb 13, 2018

A little bit of this is part of #10383.

@anntzer
Copy link
Contributor Author

anntzer commented Feb 13, 2018

seems easy to rebase in either direction

@anntzer
Make the filternorm prop of Images a boolean rather than a {0,1} scalar.
8e68152 
This seems a bit more pythonic and the C-API handles bools just as well.
Also filternorm and filterrad are always defined (as bool and float,
respectively, so no need to add `... or 0.0` when passing them to the
C-API.
@anntzer anntzer mentioned this pull request Apr 8, 2018
Merged
jklymak
jklymak reviewed Apr 8, 2018
View reviewed changes
lib/matplotlib/axes/_axes.py
filternorm : scalar, optional, default: 1
A parameter for the antigrain image resize filter. From the
antigrain documentation, if `filternorm` = 1, the filter
filternorm : bool, optional, default: True
Copy link
Member

Choose a reason for hiding this comment

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

OK, so filternorm was either 1 or zero?

The rest of this docstring doesn't make a lick of sense to me. I think they are trying to say the sum of the filter should be one, but???

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Feel free to propose a better wording :-) (it doesn't make sense to me either)
At least since #10383 matplotlib._image.resample also documents norm as a bool.

@timhoffm
Copy link
Member

timhoffm commented Apr 8, 2018 •
edited
Loading

Concerning removal of ... or 0.0:

What if users have used filterrad=False or filterrad=0? Until now, the or has silently cast them to float. Do it still work? At least the second one should.

@anntzer
Copy link
Contributor Author

anntzer commented Apr 8, 2018

set_filterrad has a check:

    def set_filterrad(self, filterrad)
        """```
        Set the resize filter radius only applicable to some
        interpolation schemes -- see help for imshow

        ACCEPTS: positive float
        """
        r = float(filterrad)
        if r <= 0:
            raise ValueError("The filter radius must be a positive number")
        self._filterrad = r
        self.stale = True

so the proposed case cannot occur.

@QuLogic QuLogic merged commit e18af55 into matplotlib:master Apr 11, 2018
@anntzer anntzer deleted the filternorm branch April 11, 2018 02:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jklymak jklymak jklymak left review comments

@QuLogic QuLogic QuLogic approved these changes

@timhoffm timhoffm timhoffm approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v3.0.0
Development

Successfully merging this pull request may close these issues.
4 participants
@anntzer @QuLogic @timhoffm @jklymak