Skip to content

Validate inputs to ScalarMappable constructor#13535

Merged
jklymak merged 1 commit into
matplotlib:masterfrom
anntzer:normcheck
Feb 11, 2020
Merged

Validate inputs to ScalarMappable constructor#13535
jklymak merged 1 commit into
matplotlib:masterfrom
anntzer:normcheck

Conversation

@anntzer
Copy link
Copy Markdown
Contributor

@anntzer anntzer commented Feb 27, 2019 •
edited
Loading

... by moving it to ScalarMappable.set_norm.

The original intent of this PR (deduplicating isinstance checks) was implemented in #14470, the only remaining point is to also perform checks in __init__.

Closes #14669.

PR Summary

PR Checklist

  • Has Pytest style unit tests
  • Code is Flake 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 force-pushed the normcheck branch 3 times, most recently from 6e2e2c8 to d07d4b3 Compare February 28, 2019 01:08
timhoffm
timhoffm requested changes Feb 28, 2019
View reviewed changes
Comment thread lib/matplotlib/cm.py Outdated
@anntzer anntzer mentioned this pull request Jul 2, 2019
Closed
@anntzer anntzer changed the title Deduplicate check that norm argument is instance of Normalize or None. Validate inputs to ScalarMappable constructor Jul 2, 2019
@dstansby
Copy link
Copy Markdown
Member

dstansby commented Jul 13, 2019

Looks 👍 - could you add a test to check that the issue described in #14669 raises a sensible warning (presumably whatever the new warning is with this PR)

@anntzer
Copy link
Copy Markdown
Contributor Author

anntzer commented Jul 13, 2019

I don't really care enough to bother, but you can push one :)

@anntzer anntzer closed this Dec 10, 2019
@anntzer anntzer deleted the normcheck branch December 10, 2019 17:58
@anntzer anntzer restored the normcheck branch December 10, 2019 18:20
@anntzer anntzer reopened this Dec 10, 2019
@jklymak
Copy link
Copy Markdown
Member

jklymak commented Dec 30, 2019

I see what you are doing in this PR, but I'm not following why its necessary. The commit message looks obsolete?

@anntzer
Copy link
Copy Markdown
Contributor Author

anntzer commented Dec 30, 2019 •
edited
Loading

Modifying the example from #14669, with

from pylab import *
plt.colorbar(mpl.cm.ScalarMappable(norm=1))  # standalone colorbar
plt.show()

Before this PR:

Traceback (most recent call last):
  File "/tmp/test.py", line 2, in <module>
    plt.colorbar(mpl.cm.ScalarMappable(norm=1))
  File ".../matplotlib/pyplot.py", line 1935, in colorbar
    ret = gcf().colorbar(mappable, cax=cax, ax=ax, **kw)
  File ".../matplotlib/figure.py", line 2219, in colorbar
    cb = cbar.colorbar_factory(cax, mappable, **cb_kw)
  File ".../matplotlib/colorbar.py", line 1670, in colorbar_factory
    cb = Colorbar(cax, mappable, **kwargs)
  File ".../matplotlib/colorbar.py", line 1195, in __init__
    ColorbarBase.__init__(self, ax, **kw)
  File ".../matplotlib/colorbar.py", line 471, in __init__
    self.draw_all()
  File ".../matplotlib/colorbar.py", line 494, in draw_all
    self._process_values()
  File ".../matplotlib/colorbar.py", line 935, in _process_values
    if not self.norm.scaled():
AttributeError: 'int' object has no attribute 'scaled'

After:

Traceback (most recent call last):
  File "/tmp/test.py", line 2, in <module>
    plt.colorbar(mpl.cm.ScalarMappable(norm=1))
  File ".../matplotlib/cm.py", line 172, in __init__
    self.set_norm(norm)  # The Normalize instance of this ScalarMappable.
  File ".../matplotlib/cm.py", line 337, in set_norm
    cbook._check_isinstance((colors.Normalize, None), norm=norm)
  File ".../matplotlib/cbook/__init__.py", line 2135, in _check_isinstance
    raise TypeError(
TypeError: 'norm' must be an instance of matplotlib.colors.Normalize or None, not a int

Also updated commit message.

tacaswell
tacaswell approved these changes Feb 11, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@tacaswell tacaswell left a comment

Choose a reason for hiding this comment

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

modulo the flake8 fix.

Anyone can merge on green.

@anntzer
Copy link
Copy Markdown
Contributor Author

anntzer commented Feb 11, 2020

should be fixed now

@jklymak
Copy link
Copy Markdown
Member

jklymak commented Feb 11, 2020

Is the travis 3.8 error just something flakey in PIL?

@anntzer
Copy link
Copy Markdown
Contributor Author

anntzer commented Feb 11, 2020

looks so to me? restarted it just to be sure...

@jklymak jklymak merged commit 7c656c8 into matplotlib:master Feb 11, 2020
@anntzer anntzer deleted the normcheck branch February 11, 2020 18:40
@QuLogic QuLogic added this to the v3.3.0 milestone Feb 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tacaswell tacaswell tacaswell approved these changes

@timhoffm timhoffm timhoffm approved these changes

Assignees

No one assigned

Labels

API: argument checking

Projects

None yet

Milestone

v3.3.0

Development

Successfully merging this pull request may close these issues.

cm.ScalarMappable should fail early when norm input is wrong

6 participants

@anntzer @dstansby @jklymak @tacaswell @timhoffm @QuLogic