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.

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

Bug in Axes.relim when the first line is y_isdata=False and possible fix #854

Closed
wants to merge 1,135 commits into from

Conversation

mdboom
Copy link
Member

@mdboom mdboom commented May 28, 2013

You can reproduce the bug with:

from matplotlib import pyplot
ax = pyplot.axes()
ax.axvline(0.5)
ax.plot([-0.1, 0, 0.2, 0.1])
ax.relim()
ax.autoscale()
(ymin, ymax) = ax.get_ylim()
print ymin, ymax
pyplot.show()

Printed ylim is -0.2 1.0 where the upper bound should be something like 0.25. You will see that there is big space between data line and the top bound.

To workaround this problem, I defined the following functions:

def _update_line_limits(self, line, updatex=True, updatey=True):
    p = line.get_path()
    if p.vertices.size > 0:
        self.dataLim.update_from_path(p, self.ignore_existing_data_limits,
                                      updatex=updatex and line.x_isdata,
                                      updatey=updatey and line.y_isdata)
        if (updatex and line.x_isdata) or (updatey and line.y_isdata):
            self.ignore_existing_data_limits = False


def relim(self):
    self.dataLim.ignore(True)
    for (udatex, updatey) in [[True, False], [False, True]]:
        self.ignore_existing_data_limits = True
        for line in self.lines:
            _update_line_limits(self, line, udatex, updatey)

    for p in self.patches:
        self._update_patch_limits(p)

If you call relim(ax) instead of ax.relim(), printed ylim is -0.1 0.25. I just simply separate the update of the line limits for x and y axis. I think you can simply replace the two functions with Axes.relim and Ax._update_line_limits.

I have currently have no time for preparing executable development version of matplotlib and its test environment. I will send a pull request at some point when I am ready, but if somebody can "pull" the snippet above and test it, please do that.

@ghost ghost assigned pelson Aug 19, 2012
@pelson
Copy link
Member

pelson commented Aug 19, 2012

This is still broken even with #731. Once that is in, I will take a closer look at this.

@pelson
Copy link
Member

pelson commented Sep 4, 2012

There is a deeper problem here. Notice how the following code results in different results:

>>> import matplotlib.pyplot as plt
>>> ax = plt.axes()
>>> plt.axvline(0.5)
>>> plt.plot([-0.1, 0, 0.2, 0.1])
>>> print ax.viewLim
Bbox('array([[ 0. , -0.2],\n       [ 3. ,  1. ]])')
>>> import matplotlib.pyplot as plt
>>> ax = plt.axes()
>>> plt.plot([-0.1, 0, 0.2, 0.1])
>>> plt.axvline(0.5)
>>> print ax.viewLim
Bbox('array([[ 0.  , -0.1 ],\n       [ 3.  ,  0.25]])')

Essentially, the problem comes down to the fact that the axes.ignore_existing_data_limits is not split into ignore x and ignore_y components. Unfortunately I don't think we can get a solution to this before the 1.2.x rc1, not least because it will need a change to some of the underlying cpp code in _path.cpp.

I should add, I don't think your ax.relim(); ax.autoscale() are strictly necessary. This should be what you get in the first place (at least, I can't get it to make any difference).

EDIT: Fixed attached code.

@dmcdougall
Copy link
Member

@pelson I'm struggling to see the difference in those two pieces of code.

@pelson
Copy link
Member

pelson commented Sep 4, 2012

Doh! I have edited it now.

mdboom and others added 25 commits April 30, 2013 11:09
…on transform when doing spine.set_position(). Also includes a testcase for the data locations.
…2, which fixes a problem with alpha-blending partially transparent backgrounds.
Fixed background colour of PNGs saved with a non-zero opacity.
Rotated text element misalignment in Agg
…oblem

Fixed failing bbox_inches='tight' case when a contour collection is empty
…ll be rendered as black.

Also fixes test case result that hit this bug
Specifically, alters the base, AGG, PDF, PGF, SVG, Cairo, and Mac OS X
backends to better enable the use of RGBA color values for both fills
and edges. An explicit alpha attribute, if set, will override the
alpha channels of the color values.

Updates test results, which are now what would be expected.

Also fixes a couple bugs with handling of linestyles.
Imsave now preserves the alpha channel.
Conflicts:
	.travis.yml
	lib/matplotlib/tests/baseline_images/test_png/pngsuite.png
	lib/matplotlib/tests/test_axes.py
@mdboom
Copy link
Member

mdboom commented May 28, 2013

I think the problem is deeper than @pelson suggests. _path.update_path_extents is already able to ignore an axis simply by putting the values out of order (i.e. (max, min) rather than (min, max)) for that axis. This works as designed (even if it's not the cleanest way to do things).

The problem here is that the "default" data range is (0, 1) on both axes. So when you start with a vline, it (correctly) ignores the y axis, and sets the xrange to (0.49, 0.51) and leaves the yrange as (0, 1). The subsequent auto limit operation on the line plot does not know that the previous y limits aren't meaningful, and therefore keeps them as they are (since they don't need to be expanded to show the data).

I think the fix here is to update relim and the Axes.__init__ constructor to start with the null data range rather than the unit data range. Then we know when a range isn't meaningful because it hasn't already been set to something finite.

I've attached a patch against 1.2.x (since this bug exists there, too), but I'm not entirely sure it isn't too big/risky for 1.2. I'd like to get this in for 1.3, however.

The attached PR also fixes #2061.

@@ -779,6 +779,16 @@ def unit():
"""
return Bbox(Bbox._unit_values.copy())

_null_values = np.array([[np.inf, np.inf], [-np.inf, -np.inf]], np.float_)
Copy link
Member

Choose a reason for hiding this comment

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

I can't see much point in defining this outside of the null staticmethod. Is there a performance reason?
Is there a reason for np.float_ rather than np.float or the more explicit np.float64?

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably not -- just being symmetrical with unit.

@pelson
Copy link
Member

pelson commented May 29, 2013

I've attached a patch against 1.2.x (since this bug exists there, too), but I'm not entirely sure it isn't too big/risky for 1.2. I'd like to get this in for 1.3, however.

I don't think this can go into v1.2.x, the change is functional (admittedly it was unintentional behaviour in the first place).

The log scale test result is very different - its hard to know which one is expected, but it is without a doubt different behaviour. Would you mind providing your analysis on the test and why it was wrong?

Other than that, this is a nice solution - though I have to admit I'm not too fussed whether it makes it into the v1.3.x series or not.

Nice work @mdboom!

@mdboom
Copy link
Member

mdboom commented May 29, 2013

The log scale test adds a axvline and axhline, both at a position 24.1. In the old version, the resulting limits were (0, 100), since the initial limits on a plot were (0, 1), and the ax?line call added to it. In the new version, the resulting limits are (10, 100), since it should only include the decades around the given value. I think this is the correct behavior.

Yeah -- I think I agree about not including this in 1.2.x. I'll rebase and create a new PR for 1.3.x, also incorporating your other comments.

@pelson
Copy link
Member

pelson commented May 29, 2013

Yeah -- I think I agree about not including this in 1.2.x. I'll rebase and create a new PR for 1.3.x, also incorporating your other comments.

Great. And thanks for the explanation - I'm more comfortable with the change now. The only remaining work is to add an API change entry for the change.

Cheers,

@mdboom mdboom mentioned this pull request May 29, 2013
@mdboom
Copy link
Member

mdboom commented May 29, 2013

Closing in favor of #2082.

@mdboom mdboom closed this May 29, 2013
@mdboom mdboom deleted the data_limits branch August 7, 2014 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet