/matplotlib

Line2D._path obeys drawstyle. #6497

Merged
merged 2 commits into from May 29, 2016

Conversation

Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
  2.1 (next point r...
3 participants
@anntzer @tacaswell @mdboom
@anntzer
Contributor

anntzer commented May 29, 2016

For stepping drawstyles, Line2D.draw used to recompute a path at drawing time, because its _path attribute always assumed a linear drawstyle. Instead, directly compute the correct path.

Also fixes #6447 (Line2D.contains did not take drawstyle into account) because that code relied on proximity of the mouse event with the underlying path.

Note that unfortunately, the drawstyle names remain inconsistent with those accepted by fill_between and fill_betweenx, which (seem to be the only functions to) rely on cbook.STEP_LOOKUP_MAP. It may be a good opportunity to fix the discrepancy too...
@anntzer anntzer Line2D._path obeys drawstyle. 
For stepping drawstyles, `Line2D.draw` used to recompute a path at
drawing time, because its `_path` attribute always assumed a linear
drawstyle.  Instead, directly compute the correct path.

Also fixes #6447 (`Line2D.contains` did not take drawstyle into account)
because that code relied on proximity of the mouse event with the
underlying path.

Some checks were not successful

1 failing and 2 successful checks
coverage/coveralls — Coverage decreased (-0.002%) to 69.771%
Details
continuous-integration/appveyor/pr — AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
722f405

mdboom added the needs_review label May 29, 2016

tacaswell added this to the 2.1 (next point release) milestone May 29, 2016

@tacaswell tacaswell commented on an outdated diff May 29, 2016

View full outdated diff
lib/matplotlib/lines.py
drawStyles = {}
drawStyles.update(_drawStyles_l)
drawStyles.update(_drawStyles_s)
# Need a list ordered with long names first:
drawStyleKeys = (list(six.iterkeys(_drawStyles_l)) +
list(six.iterkeys(_drawStyles_s)))
+ _drawstyle_conv = {
+ 'default': lambda x, y: (x, y),
+ 'steps': pts_to_prestep,
@tacaswell

tacaswell May 29, 2016

Owner

Can you just add these two to STEP_LOOKUP_MAP and correct the spelling of step-pre and friends to steps-*? I suspect I meant to do the refactor you are doing now as part of the work where the fill_between feature got added, but ran out of steam (it was one of the last things to get merged before 1.5.0 iirc).
@tacaswell
Owner

tacaswell commented May 29, 2016

What the input to fill_between and fill_betweenx is matching is the input to ax.step (see #4433).

I would keep the draw_style values as locked down as possible, but let where (in step) and step (in fill_between take the version both with and without steps).
@anntzer
Contributor

anntzer commented May 29, 2016

Like in this version?

@tacaswell tacaswell and 1 other commented on an outdated diff May 29, 2016

View full outdated diff
lib/matplotlib/cbook.py
'post': pts_to_poststep,
'mid': pts_to_midstep,
'step-pre': pts_to_prestep,
'step-post': pts_to_poststep,
- 'step-mid': pts_to_midstep}
+ 'step-mid': pts_to_midstep,
@tacaswell

tacaswell May 29, 2016

Owner

I would remove the step-* entries. They are not documented and this is the only place those strings appear in the code base. I am 98% sure the lack of 's' here was my mistake.

@anntzer

anntzer May 29, 2016

Contributor

Done.

@tacaswell tacaswell commented on the diff May 29, 2016

View full changes
lib/matplotlib/lines.py
@@ -470,8 +470,7 @@ def contains(self, mouseevent):
# application has set the error flags such that an exception is raised
# on overflow, we temporarily set the appropriate error flags here and
# set them back when we are finished.
- olderrflags = np.seterr(all='ignore')
- try:
+ with np.errstate(all='ignore'):
@tacaswell

tacaswell May 29, 2016

Owner

This is a pre-1.6 addition to numpy correct? (we are testing 1.6 with py2.7 on travis so this should be fine, but just being mildly paranoid)

@anntzer

anntzer May 29, 2016 • edited

Contributor

Yes.

$ git blame numpy/core/numeric.py | grep 'class errstate'
cf3eb93b numpy/core/numeric.py (Travis Oliphant       2006-10-11 23:52:53 +0000 2929) class errstate(object):
@anntzer anntzer Fix and use STEP_LOOKUP_MAP, for consistency.

All checks have passed

3 successful checks
continuous-integration/appveyor/pr — AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
coverage/coveralls — Coverage increased (+0.0001%) to 69.773%
Details
b8fe72c
@tacaswell
Owner

tacaswell commented May 29, 2016

👍 Will merge once CI finishes.

@tacaswell tacaswell merged commit a260058 into matplotlib:master May 29, 2016
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.0001%) to 69.773%
Details

tacaswell removed the needs_review label May 29, 2016

anntzer deleted the anntzer:fix-line2d-contains-drawstyle branch May 29, 2016

This was referenced May 30, 2016

Closed
Closed

@anntzer anntzer added a commit to anntzer/matplotlib that referenced this pull request Jun 26, 2016

@anntzer anntzer Fix containment and subslice optim. for steps. 
 #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 #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.
 13b4555

This was referenced Jun 26, 2016

Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment