Skip to content

Bad event index for step plots #6615

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

Closed
anntzer opened this issue Jun 20, 2016 · 3 comments
Closed

Bad event index for step plots #6615

anntzer opened this issue Jun 20, 2016 · 3 comments
Milestone

Comments

@anntzer
Copy link
Contributor

anntzer commented Jun 20, 2016

#6497 (checked by bisection) let to pick events reporting incorrect events for step plots.

MWE:

from pylab import *
plot(rand(100), picker=5, drawstyle="steps-pre")  # 5 points tolerance
def on_pick(event):
    print(event.ind)
cid = gcf().canvas.mpl_connect('pick_event', on_pick)
show()

On 1.5.1, clicking on the "right" part of the plot reports an index close to 100 (that is, the number of points). On master, the index is instead close to 200 (because each point is "duplicated" in the path).

While the previous behavior could "easily" be restored, it may be a good time to revisit what the "index" returned by Line2D.contains actually means. Specifically:

  • For non-step plots, I think the indices should actually be floats, that indicate where between which two points the projection of the click onto the line falls (for each line for which the projection is close enough), and how close to each extremity of the segment. (This would be only a minor backwards incompatibility if the returned index is used, well, to index the data: recent numpys would emit a warning about indexing with a float.)
  • For step plots, it is less clear what the correct solution is. Perhaps keeping the status quo and returning the index of the preceding point(s) would be enough.
@tacaswell
Copy link
Member

I am in favor of adding a event.f_index in parallel to ind and leaving ind as it is.

step should probably map back to the orginal index?

@tacaswell tacaswell added this to the 2.1 (next point release) milestone Jun 21, 2016
@anntzer
Copy link
Contributor Author

anntzer commented Jun 21, 2016

Turns out #6497 has an additional issue, where lines of length >1000 ignore the drawstyle due to the subslicing optimization

    def _transform_path(self, subslice=None):
        # <...>
        if subslice is not None:
            _steps = self._path._interpolation_steps
            _path = Path(self._xy[subslice, :], _interpolation_steps=_steps)

A call to STEP_LOOKUP_MAP[self._drawstyle] is missing here; however just adding it is not sufficient to solve the issue because the subslice indices are wrong (try zooming in to see what I mean).

Either the subslicing indices should be fixed, or possibly (the simpler solution?) we can just revert #6497, rely on the previous method on plotting, and cache the actually plotted path under a new name for containment checking.

anntzer added a commit to anntzer/matplotlib that referenced this issue Jun 26, 2016
 matplotlib#6497 set the `_path` attribute of Line2D to the actually drawn path
even if the drawstyle is `steps-*`; however containment tests and the
subslice optimization for very long paths were not updated accordingly
(see matplotlib#6615).  This patch fixes the issues.

Note that `contains` returns, for events in a horizontal segment of a
"steps-mid" drawstyle plot, the index of the point in the segment,
regardless of whether the event occured to the left or the right of
that point.
@anntzer
Copy link
Contributor Author

anntzer commented Jul 14, 2016

Closed by #6645.

@anntzer anntzer closed this as completed Jul 14, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants