Skip to content

Call _transform_vmin_vmax during SymLogNorm.__init__ - #6780

Merged
tacaswell merged 4 commits into
matplotlib:masterfrom
aWinglessMonkey:Salganos-SymLogNorm
Dec 20, 2016
Merged

Call _transform_vmin_vmax during SymLogNorm.__init__#6780
tacaswell merged 4 commits into
matplotlib:masterfrom
aWinglessMonkey:Salganos-SymLogNorm

Conversation

@aWinglessMonkey

@aWinglessMonkey aWinglessMonkey commented Jul 17, 2016

Copy link
Copy Markdown
Contributor

A fix for #6750

@tacaswell tacaswell added this to the 2.0.1 (next bug fix release) milestone Jul 17, 2016
@tacaswell

tacaswell commented Jul 23, 2016

Copy link
Copy Markdown
Member

👍

Can you also add a test? It does not need to be an image test, just the minimal code that would have raised before.

@aWinglessMonkey

aWinglessMonkey commented Jul 25, 2016 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Not sure where to add that, but I'll look into it. The minimal test should be something like:
cb.ColorbarBase(plt.figure().add_subplot(111), norm = clr.SymLogNorm(linthresh = .1, vmin = -1, vmax = 1))

Edit: I think I've found the spot.

tacaswell
tacaswell reviewed Jul 25, 2016
View reviewed changes
Comment thread lib/matplotlib/tests/test_colors.py Outdated
normed_vals = norm(vals)
assert_array_almost_equal(normed_vals, expected)

# Ensure that a SymLogNorm object can be used in a colorbar

@tacaswell tacaswell Jul 25, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This should be it's own test (any function name that starts with test_ will work).

It also needs the @cleanup decorator.

@tacaswell

tacaswell commented Jul 26, 2016

Copy link
Copy Markdown
Member

❤️ pep8/pycodestyle

There is some trailing white space:

FAIL: matplotlib.tests.test_coding_standards.test_pep8_conformance_installed_files
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/build/matplotlib/matplotlib/venv/lib/python3.5/site-packages/nose/case.py", line 198, in runTest
    self.test(*self.arg)
  File "/home/travis/build/matplotlib/matplotlib/lib/matplotlib/tests/test_coding_standards.py", line 244, in test_pep8_conformance_installed_files
    expected_bad_files=expected_bad_files)
  File "/home/travis/build/matplotlib/matplotlib/lib/matplotlib/tests/test_coding_standards.py", line 143, in assert_pep8_conformance
    assert_equal(result.total_errors, 0, msg)
AssertionError: 1 != 0 : Found code syntax errors (and warnings):
/home/travis/build/matplotlib/matplotlib/lib/matplotlib/tests/test_colors.py:226:1: W293 blank line contains whitespace

This may seem petty and annoying, but keeping a consistent style (including no trailing white space) really does help on big projects with many contributors.

@aWinglessMonkey

aWinglessMonkey commented Jul 26, 2016

Copy link
Copy Markdown
Contributor Author

Whoops. The perils of not using a proper development environment.

@NelleV

NelleV commented Dec 19, 2016

Copy link
Copy Markdown
Member

👍 LGTM

@NelleV NelleV changed the title Call _transform_vmin_vmax during SymLogNorm.__init__ [MRG+1] Call _transform_vmin_vmax during SymLogNorm.__init__ Dec 19, 2016
efiring
efiring approved these changes Dec 19, 2016
View reviewed changes

@efiring efiring left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Apart from the minor suggestion to delete a line from the test, this looks good--it makes sense.

Comment thread lib/matplotlib/tests/test_colors.py
norm = mcolors.SymLogNorm(0.1, vmin=-1, vmax=1, linscale=1)
fig = plt.figure()
cbar = mcolorbar.ColorbarBase(fig.add_subplot(111), norm=norm)
plt.close(fig)

@efiring efiring Dec 19, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this close is unnecessary; the @cleanup decorator handles it, doesn't it?

@tacaswell tacaswell Dec 20, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It does, but this does not hurt

@NelleV NelleV changed the title [MRG+1] Call _transform_vmin_vmax during SymLogNorm.__init__ [MRG+2] Call _transform_vmin_vmax during SymLogNorm.__init__ Dec 19, 2016
@tacaswell
tacaswell merged commit 58a8334 into matplotlib:master Dec 20, 2016
@tacaswell tacaswell modified the milestones: 2.1 (next point release), 2.0.1 (next bug fix release) Dec 20, 2016
@tacaswell

tacaswell commented Dec 20, 2016

Copy link
Copy Markdown
Member

Changed my mind about targeting 2.x . If someone disagrees, I will not protest to it being backported.

@efiring

efiring commented Dec 20, 2016

Copy link
Copy Markdown
Member

It's not critical, but I am mildly in favor of the backport. The change has no effect in most uses, but blocks a potential failure mode.

@QuLogic

QuLogic commented Dec 26, 2016

Copy link
Copy Markdown
Member

@efiring Are you going to backport?

@efiring

efiring commented Dec 26, 2016

Copy link
Copy Markdown
Member

Sure, I can do that in a few minutes.

efiring pushed a commit that referenced this pull request Dec 26, 2016
@tacaswell @efiring
Merge pull request #6780 from Salganos/Salganos-SymLogNorm
0ab4599 
ENH: Call _transform_vmin_vmax during SymLogNorm.__init__
@efiring

efiring commented Dec 26, 2016

Copy link
Copy Markdown
Member

Backported to v2.x as 0ab4599

@QuLogic QuLogic modified the milestones: 2.0 (style change major release), 2.1 (next point release) Dec 26, 2016
@QuLogic QuLogic changed the title [MRG+2] Call _transform_vmin_vmax during SymLogNorm.__init__ Call _transform_vmin_vmax during SymLogNorm.__init__ Dec 26, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tacaswell tacaswell tacaswell left review comments
@efiring efiring efiring approved these changes
@NelleV NelleV NelleV approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

v2.0.0

Development

Successfully merging this pull request may close these issues.

6 participants

@aWinglessMonkey @tacaswell @NelleV @efiring @QuLogic @mdboom