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.

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

Call _transform_vmin_vmax during SymLogNorm.__init__ #6780

Merged
merged 4 commits into from
Dec 20, 2016
Merged

Call _transform_vmin_vmax during SymLogNorm.__init__ #6780

merged 4 commits into from
Dec 20, 2016

Conversation

aWinglessMonkey
Copy link
Contributor

A fix for #6750

@tacaswell tacaswell added this to the 2.0.1 (next bug fix release) milestone Jul 17, 2016
@tacaswell
Copy link
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
Copy link
Contributor Author

aWinglessMonkey commented Jul 25, 2016 •
edited
Loading

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
lib/matplotlib/tests/test_colors.py
@@ -222,6 +223,13 @@ def test_SymLogNorm():
norm = mcolors.SymLogNorm(3, vmin=-30, vmax=5, linscale=1.2)
normed_vals = norm(vals)
assert_array_almost_equal(normed_vals, expected)

# Ensure that a SymLogNorm object can be used in a colorbar
Copy link
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
Copy link
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
Copy link
Contributor Author

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

@NelleV
Copy link
Member

NelleV commented Dec 19, 2016

👍 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
Copy link
Member

@efiring efiring left a comment

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.

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)
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 this close is unnecessary; the @cleanup decorator handles it, doesn't it?

Copy link
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
Copy link
Member

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

@efiring
Copy link
Member

efiring commented Dec 20, 2016

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
Copy link
Member

QuLogic commented Dec 26, 2016

@efiring Are you going to backport?

@efiring
Copy link
Member

efiring commented Dec 26, 2016

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
Copy link
Member

efiring commented Dec 26, 2016

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
No open projects
Reviewing pull requests.
Done
Milestone
v2.0.0
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@aWinglessMonkey @tacaswell @NelleV @efiring @QuLogic @mdboom