Fix typo in Axes3D.set_autoscalez_on. #7859

anntzer · 2017-01-17T18:08:34Z

(See surrounding code to confirm that the correct attribute name is
indeed _autoscaleZon.)

Split out of #5538.

NelleV · 2017-01-17T18:10:36Z

wow… nice catch.
Is it possible to add a test for this one?

WeatherGod · 2017-01-17T18:11:58Z

+1 from me barring any issues raised by CI

WeatherGod · 2017-01-17T18:13:32Z

I am a bit concerned that this hasn't been noticed for so long. I would have expected an attribute error if this codepath was actually active

NelleV · 2017-01-17T18:14:28Z

Well… The correct attribute is used in the code, so I think this is more a case of code that doesn't crash but does not behave as expected.

anntzer · 2017-01-17T18:15:58Z

I'll skip the test myself, given the number of things that are untested in mplot3d anyways... :-)

NelleV · 2017-01-17T18:20:45Z

Yeah… I am not 100% convinced this is the right strategy :)

WeatherGod · 2017-01-17T18:27:26Z

The setter is one attribute, while the getter is a different attribute, so I would expect an attribute error

WeatherGod · 2017-01-17T18:30:00Z

ah, set_zlim3d() is setting the other attribute directly instead of using the setter, so that's how the attribute error is getting avoided

WeatherGod · 2017-01-17T20:57:46Z

failures are unrelated (one of them was a failure for git clean to remove result_images/index.html for some reason?)

NelleV · 2017-01-18T00:11:27Z

Should we start backporting?

tacaswell · 2017-01-18T01:59:04Z

Yes 👍 to backporting.

anntzer · 2017-01-18T05:44:32Z

@NelleV Now can I get a coffee instead?
(I did catch another bug -- Axes3D.margins would error if passed three keyword arguments...)

anntzer · 2017-01-18T07:43:07Z

eh... something unrelatedly wrong happened to the tests?

======================================================================
ERROR: test suite for <function test_divider_append_axes at 0x7fffe8d450c8>
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/build/matplotlib/matplotlib/venv/lib/python2.7/site-packages/nose/plugins/multiprocess.py", line 788, in run
    self.setUp()
  File "/home/travis/build/matplotlib/matplotlib/venv/lib/python2.7/site-packages/nose/suite.py", line 292, in setUp
    self.setupContext(ancestor)
  File "/home/travis/build/matplotlib/matplotlib/venv/lib/python2.7/site-packages/nose/plugins/multiprocess.py", line 770, in setupContext
    super(NoSharedFixtureContextSuite, self).setupContext(context)
  File "/home/travis/build/matplotlib/matplotlib/venv/lib/python2.7/site-packages/nose/suite.py", line 315, in setupContext
    try_run(context, names)
  File "/home/travis/build/matplotlib/matplotlib/venv/lib/python2.7/site-packages/nose/util.py", line 490, in try_run
    return func()
  File "/home/travis/build/matplotlib/matplotlib/lib/matplotlib/testing/decorators.py", line 268, in setup
    ' (`%s`)' % getattr(func, '__qualname__', func.__name__))
AssertionError: Figures and baseline_images count are not the same (`test_divider_append_axes`)

jenshnielsen · 2017-01-18T08:29:57Z

I think your test needs a cleanup decorator otherwise that figure will leak into other tests.

anntzer · 2017-01-18T08:37:07Z

Now everyone knows I don't know how to write a test...

codecov-io · 2017-01-18T11:16:38Z

Current coverage is 62.16% (diff: 86.36%)

Merging #7859 into master will increase coverage by 0.04%

@@             master      #7859   diff @@
==========================================
  Files           174        174          
  Lines         56072      56072          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          34833      34857    +24   
+ Misses        21239      21215    -24   
  Partials          0          0

Powered by Codecov. Last update 2374c9b...a437517

WeatherGod · 2017-01-18T16:51:04Z

lib/mpl_toolkits/mplot3d/axes3d.py

            mx, my = args
        elif len(args) == 3:
            mx, my, mz = args
        else:
-            raise ValueError("more than three arguments were supplied")
+            raise TypeError(
+                "Axes3D.margins takes at most three positional arguments")


I raised a ValueError here originally because that what the 2d Axes.margins() does when given more than two arguments.

May or may not be a gratuitious backwards compatible change but arguments mismatching the signature normally raise a TypeError. Should we change it in Axes.margins too? (hem hem)

along the same line of thinking, wouldn't the 2d version of this function suffer the same problem that you are trying to fix here with an empty args list?

Nope. Haven't checked the impl. but it must have been fixed at some point.

just took a look, it seems like the 2d version doesn't have this else-clause, so it silently allows more arguments than expected.

Open a separate issue? :)

grrr, misread the code... it doesn't have an else clause... it has a "len(args) > 2" clause that then raises the ValueError, but also skips the case of no positional args. We are fine, then.

As for ValueError vs. TypeError, I really don't have a preference except that the 2d and 3d version be the same.

WeatherGod · 2017-01-18T16:52:34Z

lib/mpl_toolkits/mplot3d/axes3d.py

-                scaley = self._autoscaleYon
+                self._autoscaleYon = scaley = bool(enable)
+            else:
+                scaly = False


typo, plus, the else-clause isn't needed as the scale{x,y,z} variables are already initialized to False

Forgot to remove the initialization. Fixed now; I think this is more readable.

WeatherGod · 2017-01-18T16:54:44Z

lib/mpl_toolkits/tests/test_mplot3d.py

@@ -469,6 +469,19 @@ def test_lines_dists():
    ax.set_ylim(0, 300)


+


too many whitespace lines

NelleV · 2017-01-18T18:16:08Z

@anntzer You've earned your coffee. Let me know when and where you want it :)

WeatherGod · 2017-01-23T17:57:37Z

This will need a rebase due to the pytest migration. Plus, the remaining sticking point about the exception raised hasn't been resolved. I honestly don't care which one we raise, but they should be the same for both 2D and 3D. We could make a change for it to be in a separate PR, as it really is out of scope here.

NelleV · 2017-01-27T21:09:45Z

@anntzer Do you mind rebasing and (/or) fixing @WeatherGod 's very minor one-line of change comment?

(See surrounding code to confirm that the correct attribute name is indeed `_autoscaleZon`.)

anntzer · 2017-01-27T22:50:37Z

Rebased and made it a ValueError instead as per @WeatherGod's suggestion. Will open a separate issue for that.

NelleV · 2017-01-28T19:06:16Z

Thanks @anntzer

QuLogic · 2017-01-28T23:43:42Z

Needs backport?

anntzer · 2017-01-29T00:34:33Z

I wouldn't bother, the code has been broken basically forever without anyone noticing (channeling @NelleV here :p).

NelleV · 2017-01-29T03:34:45Z

I wouldn't bother either.

QuLogic · 2017-01-29T07:34:02Z

Please update the milestone if you don't plan on backporting things.

tacaswell modified the milestones: 2.1 (next point release), 2.0.1 (next bug fix release) Jan 17, 2017

tacaswell approved these changes Jan 17, 2017

View reviewed changes

tacaswell changed the title ~~Fix typo in Axes3D.set_autoscalez_on.~~ [MRG+1] Fix typo in Axes3D.set_autoscalez_on. Jan 17, 2017

anntzer force-pushed the set_autoscalez_on branch from b58614d to f050673 Compare January 18, 2017 08:31

WeatherGod requested changes Jan 18, 2017

View reviewed changes

anntzer force-pushed the set_autoscalez_on branch from f050673 to a437517 Compare January 18, 2017 17:46

anntzer added 3 commits January 27, 2017 14:24

Fix typo in Axes3D.set_autoscalez_on.

bcb7976

(See surrounding code to confirm that the correct attribute name is indeed `_autoscaleZon`.)

Whitespace cleanup of axes3d.

5347d49

Fix passing kwargs only to margins; add test.

effe642

anntzer force-pushed the set_autoscalez_on branch from a437517 to effe642 Compare January 27, 2017 22:50

NelleV merged commit ec8cf08 into matplotlib:master Jan 28, 2017

anntzer deleted the set_autoscalez_on branch January 28, 2017 19:25

QuLogic changed the title ~~[MRG+1] Fix typo in Axes3D.set_autoscalez_on.~~ Fix typo in Axes3D.set_autoscalez_on. Jan 28, 2017

QuLogic modified the milestones: 2.1 (next point release), 2.0.1 (next bug fix release) Jan 29, 2017

		@@ -469,6 +469,19 @@ def test_lines_dists():
		ax.set_ylim(0, 300)

Fix typo in Axes3D.set_autoscalez_on. #7859

Fix typo in Axes3D.set_autoscalez_on. #7859

Conversation

anntzer commented Jan 17, 2017

NelleV commented Jan 17, 2017

WeatherGod commented Jan 17, 2017

WeatherGod commented Jan 17, 2017

NelleV commented Jan 17, 2017

anntzer commented Jan 17, 2017

NelleV commented Jan 17, 2017

WeatherGod commented Jan 17, 2017

WeatherGod commented Jan 17, 2017

WeatherGod commented Jan 17, 2017

NelleV commented Jan 18, 2017

tacaswell commented Jan 18, 2017

anntzer commented Jan 18, 2017

anntzer commented Jan 18, 2017

jenshnielsen commented Jan 18, 2017

anntzer commented Jan 18, 2017

codecov-io commented Jan 18, 2017 • edited Loading

Current coverage is 62.16% (diff: 86.36%)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NelleV commented Jan 18, 2017

WeatherGod commented Jan 23, 2017

NelleV commented Jan 27, 2017

anntzer commented Jan 27, 2017

NelleV commented Jan 28, 2017

QuLogic commented Jan 28, 2017

anntzer commented Jan 29, 2017

NelleV commented Jan 29, 2017

QuLogic commented Jan 29, 2017

codecov-io commented Jan 18, 2017 •

edited

Loading