Autoscaling and limits in mplot3d. #1289

Merged
merged 4 commits into from Dec 3, 2012

Projects

None yet

3 participants

@WeatherGod
Matplotlib Developers member

Due to time constraints in releasing v1.2.0, I would understand if this doesn't make it in for the release.

  • Fixes improper setting of x,y,z limits not turning off autoscaling
    • Adds units support to z-axis
    • Adds shared axis support for z-axis
    • Closes #783
@WeatherGod WeatherGod Autoscaling and limits in mplot3d.
    * Fixes improper setting of x,y,z limits not turning off autoscaling
    * Adds units support to z-axis
    * Adds shared axis support for z-axis
    * Closes #783
e76fd7f
@dmcdougall dmcdougall and 1 other commented on an outdated diff Sep 20, 2012
lib/mpl_toolkits/mplot3d/axes3d.py
@@ -831,7 +1021,7 @@ def cla(self):
self.zaxis.cla()
# TODO: Support sharez
- self._sharez = None
+ #self._sharez = None
@dmcdougall
dmcdougall Sep 20, 2012

If z-axis sharing is supported, why not just remove these three lines?

@WeatherGod
WeatherGod Sep 20, 2012

Whoops, I thought I got all the cruft. I will take care of that.

@dmcdougall
Matplotlib Developers member

Does this suffer from the problems reported in #1110? If so, #1145 fixed it for the 2D case. Maybe it's worth taking a look at the diff there?

@WeatherGod
Matplotlib Developers member

Axes3D can't do twinx/y yet because of the way the axis and axis ticks and labels are drawn. Support for that will have to come much further down the road.

@dmcdougall
Matplotlib Developers member

Sweet. Cheers for clarification.

@pelson
Matplotlib Developers member

My biggest concern with mplot3d in general is that I don't think there are any tests? At the very least, would you be willing to add a simple test case for this change?

@WeatherGod
Matplotlib Developers member

Oh, I whole-heartedly agree that one of mplot3d's main problems is the lack of tests. I have been putting off making such tests because I have never been satisfied with the appearance of the figures. What I do whenever I make an invasive change like this is that I run all of the mplot3d examples (plus others I have) and make sure that they still produce expected results.

My one remaining hurdle I have before I am really willing to sit down and create baseline images is getting the 3d axes to take up a significantly larger portion of the subaxes. Right now, everything looks crammed. That should be the last major visual change for mplot3d toolkit. Unfortunately, it is not quite possible to do this, although some changes that have happened over the summer should get us most of the way there.

@WeatherGod
Matplotlib Developers member

Now that v1.2.0 is out, I want to merge this bugfix into the maintenance branch and master. Objections?

@dmcdougall
Matplotlib Developers member

My feeling is that this should be merged, but just into master. It's not entirely a bugfix, it adds support for things that weren't previously supported.

@WeatherGod
Matplotlib Developers member
@dmcdougall
Matplotlib Developers member

@WeatherGod Alright. I'd like to play with it first though. I haven't done that yet.

@dmcdougall
Matplotlib Developers member

@WeatherGod Tried this out -- it seems to behave like the 2D case for the example in #783. I haven't tried the units support.

How does everyone else feel about this going into the v1.2.x branch?

@dmcdougall dmcdougall and 1 other commented on an outdated diff Nov 16, 2012
lib/mpl_toolkits/mplot3d/axes3d.py
@@ -112,6 +124,44 @@ def set_axis_off(self):
def set_axis_on(self):
self._axis3don = True
+ def have_units(self):
+ 'Return *True* if units are set on the *x*, *y*, or *z* axes'
@dmcdougall
dmcdougall Nov 16, 2012

Is there any reason you didn't make this a """ docstring?

@WeatherGod
WeatherGod Nov 16, 2012

Most of the PEP8 violations here are a result of my copying and pasting from axes.py. Thanks for catching these and I will fix them all (as well as do PEP8 work for mplot3d in a separate PR).

@dmcdougall dmcdougall commented on an outdated diff Nov 16, 2012
lib/mpl_toolkits/mplot3d/axes3d.py
@@ -112,6 +124,44 @@ def set_axis_off(self):
def set_axis_on(self):
self._axis3don = True
+ def have_units(self):
+ 'Return *True* if units are set on the *x*, *y*, or *z* axes'
+ return (self.xaxis.have_units() or self.yaxis.have_units() or
+ self.zaxis.have_units())
+
+ def convert_zunits(self, z):
+ """For artists in an axes, if the zaxis has units support,
@dmcdougall
dmcdougall Nov 16, 2012

Should the For artists... part start on a new line?

@dmcdougall dmcdougall commented on an outdated diff Nov 16, 2012
lib/mpl_toolkits/mplot3d/axes3d.py
+ """
+ Look for unit *kwargs* and update the axis instances as necessary
+ """
+ Axes._process_unit_info(self, xdata=xdata, ydata=ydata, kwargs=kwargs)
+
+ if self.xaxis is None or self.yaxis is None or self.zaxis is None:
+ return
+
+ if zdata is not None:
+ # we only need to update if there is nothing set yet.
+ if not self.zaxis.have_units():
+ self.zaxis.update_units(xdata)
+
+ # process kwargs 2nd since these will override default units
+ if kwargs is not None:
+ zunits = kwargs.pop( 'zunits', self.zaxis.units)
@dmcdougall
dmcdougall Nov 16, 2012

There's a stray space between ( and '.

@dmcdougall dmcdougall commented on an outdated diff Nov 16, 2012
lib/mpl_toolkits/mplot3d/axes3d.py
+ Look for unit *kwargs* and update the axis instances as necessary
+ """
+ Axes._process_unit_info(self, xdata=xdata, ydata=ydata, kwargs=kwargs)
+
+ if self.xaxis is None or self.yaxis is None or self.zaxis is None:
+ return
+
+ if zdata is not None:
+ # we only need to update if there is nothing set yet.
+ if not self.zaxis.have_units():
+ self.zaxis.update_units(xdata)
+
+ # process kwargs 2nd since these will override default units
+ if kwargs is not None:
+ zunits = kwargs.pop( 'zunits', self.zaxis.units)
+ if zunits!=self.zaxis.units:
@dmcdougall
dmcdougall Nov 16, 2012

Could you put spaces around !=?

@dmcdougall dmcdougall commented on the diff Nov 16, 2012
lib/mpl_toolkits/mplot3d/axes3d.py
@@ -407,22 +459,17 @@ def autoscale_view(self, tight=None, scalex=True, scaley=True,
.. versionchanged :: 1.1.0
Function signature was changed to better match the 2D version.
- *tight* is now explicitly a kwarg and placed first. However,
- it currently does not do anything.
- """
+ *tight* is now explicitly a kwarg and placed first.
@dmcdougall
dmcdougall Nov 16, 2012

I thought it didn't matter where kwargs were placed. Am I not understanding something?

@WeatherGod
WeatherGod Nov 16, 2012

IIRC, the issue was that originally this function had **kwargs instead of explicit kwarg variables. The call signature did not match that from the 2d axes equivalent function. In v1.1.x, I fixed this issue.

@dmcdougall
dmcdougall Nov 19, 2012

@WeatherGod To clarify, you're going to address the PEP8 issues here (along with the rest of the mplot3d module) in a different pull request?

@WeatherGod
WeatherGod Dec 3, 2012

Completely missed this comment. I will update this PR to be PEP8 compliant, but then address the remaining PEP8 issues in mplot3d separately. In other words, I don't want to introduce new code with bad style. Fixes forthcoming.

@dmcdougall dmcdougall commented on an outdated diff Nov 16, 2012
lib/mpl_toolkits/mplot3d/axes3d.py
'''
- # TODO: Add compatibility for 'left' and 'right'
- # TODO: support 'emit' and 'auto'
- lims = self._determine_lims(*args, **kwargs)
- self.xy_viewLim.intervalx = lims
- return lims
+ if 'xmin' in kw:
+ left = kw.pop('xmin')
+ if 'xmax' in kw:
+ right = kw.pop('xmax')
+ if kw:
+ raise ValueError("unrecognized kwargs: %s" % kw.keys())
+
+ if right is None and iterable(left):
+ left,right = left
@dmcdougall
dmcdougall Nov 16, 2012

Could you put a space after the comma?

@dmcdougall dmcdougall commented on an outdated diff Nov 16, 2012
lib/mpl_toolkits/mplot3d/axes3d.py
+ if 'xmax' in kw:
+ right = kw.pop('xmax')
+ if kw:
+ raise ValueError("unrecognized kwargs: %s" % kw.keys())
+
+ if right is None and iterable(left):
+ left,right = left
+
+ self._process_unit_info(xdata=(left, right))
+ if left is not None:
+ left = self.convert_xunits(left)
+ if right is not None:
+ right = self.convert_xunits(right)
+
+ old_left, old_right = self.get_xlim()
+ if left is None: left = old_left
@dmcdougall
dmcdougall Nov 16, 2012

Could you make these blocks?

if left is None:
    left = old_left
@dmcdougall dmcdougall commented on an outdated diff Nov 16, 2012
lib/mpl_toolkits/mplot3d/axes3d.py
+ right = kw.pop('xmax')
+ if kw:
+ raise ValueError("unrecognized kwargs: %s" % kw.keys())
+
+ if right is None and iterable(left):
+ left,right = left
+
+ self._process_unit_info(xdata=(left, right))
+ if left is not None:
+ left = self.convert_xunits(left)
+ if right is not None:
+ right = self.convert_xunits(right)
+
+ old_left, old_right = self.get_xlim()
+ if left is None: left = old_left
+ if right is None: right = old_right
@dmcdougall dmcdougall commented on an outdated diff Nov 16, 2012
lib/mpl_toolkits/mplot3d/axes3d.py
+ raise ValueError("unrecognized kwargs: %s" % kw.keys())
+
+ if right is None and iterable(left):
+ left,right = left
+
+ self._process_unit_info(xdata=(left, right))
+ if left is not None:
+ left = self.convert_xunits(left)
+ if right is not None:
+ right = self.convert_xunits(right)
+
+ old_left, old_right = self.get_xlim()
+ if left is None: left = old_left
+ if right is None: right = old_right
+
+ if left==right:
@dmcdougall
dmcdougall Nov 16, 2012

Could you put spaces around ==?

@dmcdougall dmcdougall commented on an outdated diff Nov 16, 2012
lib/mpl_toolkits/mplot3d/axes3d.py
+
+ if right is None and iterable(left):
+ left,right = left
+
+ self._process_unit_info(xdata=(left, right))
+ if left is not None:
+ left = self.convert_xunits(left)
+ if right is not None:
+ right = self.convert_xunits(right)
+
+ old_left, old_right = self.get_xlim()
+ if left is None: left = old_left
+ if right is None: right = old_right
+
+ if left==right:
+ warnings.warn(('Attempting to set identical left==right results\n'
@dmcdougall
dmcdougall Nov 16, 2012

Spaces around ==.

@dmcdougall
dmcdougall Nov 16, 2012

Just realised this is in a string. You can ignore my previous comment if you wish.

@dmcdougall dmcdougall commented on an outdated diff Nov 16, 2012
lib/mpl_toolkits/mplot3d/axes3d.py
+ if right is None and iterable(left):
+ left,right = left
+
+ self._process_unit_info(xdata=(left, right))
+ if left is not None:
+ left = self.convert_xunits(left)
+ if right is not None:
+ right = self.convert_xunits(right)
+
+ old_left, old_right = self.get_xlim()
+ if left is None: left = old_left
+ if right is None: right = old_right
+
+ if left==right:
+ warnings.warn(('Attempting to set identical left==right results\n'
+ + 'in singular transformations; automatically expanding.\n'
@dmcdougall
dmcdougall Nov 16, 2012

I don't think you need to + strings if they're on different lines.

@dmcdougall dmcdougall commented on an outdated diff Nov 16, 2012
lib/mpl_toolkits/mplot3d/axes3d.py
'''
- # TODO: Add compatibility for 'top' and 'bottom'
- # TODO: support 'emit' and 'auto'
- lims = self._determine_lims(*args, **kwargs)
- self.xy_viewLim.intervaly = lims
- return lims
+ if 'ymin' in kw:
+ bottom = kw.pop('ymin')
+ if 'ymax' in kw:
+ top = kw.pop('ymax')
+ if kw:
+ raise ValueError("unrecognized kwargs: %s" % kw.keys())
+
+ if top is None and iterable(bottom):
+ bottom,top = bottom
@dmcdougall
dmcdougall Nov 16, 2012

Put a space after the ,.

@dmcdougall dmcdougall commented on an outdated diff Nov 16, 2012
lib/mpl_toolkits/mplot3d/axes3d.py
+ top = kw.pop('ymax')
+ if kw:
+ raise ValueError("unrecognized kwargs: %s" % kw.keys())
+
+ if top is None and iterable(bottom):
+ bottom,top = bottom
+
+ self._process_unit_info(ydata=(bottom, top))
+ if bottom is not None:
+ bottom = self.convert_yunits(bottom)
+ if top is not None:
+ top = self.convert_yunits(top)
+
+ old_bottom, old_top = self.get_ylim()
+ if bottom is None: bottom = old_bottom
+ if top is None: top = old_top
@dmcdougall
dmcdougall Nov 16, 2012

Could you make these two ifs blocks, please?

@dmcdougall dmcdougall commented on an outdated diff Nov 16, 2012
lib/mpl_toolkits/mplot3d/axes3d.py
+ raise ValueError("unrecognized kwargs: %s" % kw.keys())
+
+ if top is None and iterable(bottom):
+ bottom,top = bottom
+
+ self._process_unit_info(ydata=(bottom, top))
+ if bottom is not None:
+ bottom = self.convert_yunits(bottom)
+ if top is not None:
+ top = self.convert_yunits(top)
+
+ old_bottom, old_top = self.get_ylim()
+ if bottom is None: bottom = old_bottom
+ if top is None: top = old_top
+
+ if top==bottom:
@dmcdougall
dmcdougall Nov 16, 2012

Spaces around ==.

@dmcdougall dmcdougall commented on an outdated diff Nov 16, 2012
lib/mpl_toolkits/mplot3d/axes3d.py
+ bottom,top = bottom
+
+ self._process_unit_info(ydata=(bottom, top))
+ if bottom is not None:
+ bottom = self.convert_yunits(bottom)
+ if top is not None:
+ top = self.convert_yunits(top)
+
+ old_bottom, old_top = self.get_ylim()
+ if bottom is None: bottom = old_bottom
+ if top is None: top = old_top
+
+ if top==bottom:
+ warnings.warn(('Attempting to set identical bottom==top results\n'
+ + 'in singular transformations; automatically expanding.\n'
+ + 'bottom=%s, top=%s') % (bottom, top))
@dmcdougall
dmcdougall Nov 16, 2012

Don't need to add strings.

@dmcdougall dmcdougall commented on an outdated diff Nov 16, 2012
lib/mpl_toolkits/mplot3d/axes3d.py
'''
- # TODO: Add compatibility for 'top' and 'bottom'
- # TODO: support 'emit' and 'auto'
- lims = self._determine_lims(*args, **kwargs)
- self.zz_viewLim.intervalx = lims
- return lims
+ if 'zmin' in kw:
+ bottom = kw.pop('zmin')
+ if 'zmax' in kw:
+ top = kw.pop('zmax')
+ if kw:
+ raise ValueError("unrecognized kwargs: %s" % kw.keys())
+
+ if top is None and iterable(bottom):
+ bottom,top = bottom
@dmcdougall
dmcdougall Nov 16, 2012

Space after ,.

@dmcdougall dmcdougall commented on an outdated diff Nov 16, 2012
lib/mpl_toolkits/mplot3d/axes3d.py
+ top = kw.pop('zmax')
+ if kw:
+ raise ValueError("unrecognized kwargs: %s" % kw.keys())
+
+ if top is None and iterable(bottom):
+ bottom,top = bottom
+
+ self._process_unit_info(zdata=(bottom, top))
+ if bottom is not None:
+ bottom = self.convert_zunits(bottom)
+ if top is not None:
+ top = self.convert_zunits(top)
+
+ old_bottom, old_top = self.get_zlim()
+ if bottom is None: bottom = old_bottom
+ if top is None: top = old_top
@dmcdougall dmcdougall commented on an outdated diff Nov 16, 2012
lib/mpl_toolkits/mplot3d/axes3d.py
+ raise ValueError("unrecognized kwargs: %s" % kw.keys())
+
+ if top is None and iterable(bottom):
+ bottom,top = bottom
+
+ self._process_unit_info(zdata=(bottom, top))
+ if bottom is not None:
+ bottom = self.convert_zunits(bottom)
+ if top is not None:
+ top = self.convert_zunits(top)
+
+ old_bottom, old_top = self.get_zlim()
+ if bottom is None: bottom = old_bottom
+ if top is None: top = old_top
+
+ if top==bottom:
@dmcdougall dmcdougall commented on an outdated diff Nov 16, 2012
lib/mpl_toolkits/mplot3d/axes3d.py
+ bottom,top = bottom
+
+ self._process_unit_info(zdata=(bottom, top))
+ if bottom is not None:
+ bottom = self.convert_zunits(bottom)
+ if top is not None:
+ top = self.convert_zunits(top)
+
+ old_bottom, old_top = self.get_zlim()
+ if bottom is None: bottom = old_bottom
+ if top is None: top = old_top
+
+ if top==bottom:
+ warnings.warn(('Attempting to set identical bottom==top results\n'
+ + 'in singular transformations; automatically expanding.\n'
+ + 'bottom=%s, top=%s') % (bottom, top))
@dmcdougall
dmcdougall Nov 16, 2012

Adding strings.

@dmcdougall dmcdougall commented on an outdated diff Nov 16, 2012
lib/mpl_toolkits/mplot3d/axes3d.py
@@ -1958,6 +2145,9 @@ def scatter(self, xs, ys, zs=0, zdir='z', s=20, c='b', *args, **kwargs):
is_2d = False
art3d.patch_collection_2d_to_3d(patches, zs=zs, zdir=zdir)
+ if self._zmargin < 0.05 and xs.size > 0 :
@dmcdougall
dmcdougall Nov 16, 2012

Get rid of the space before :.

@WeatherGod
Matplotlib Developers member

Ok, done.

@dmcdougall
Matplotlib Developers member

Woohoooooo. Cheers mate. Another one bites the dust.

@dmcdougall dmcdougall merged commit b2a7195 into matplotlib:v1.2.x Dec 3, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment