Cleanup for drawstyles. #6564

Merged
merged 1 commit into from Jun 19, 2016

Conversation

Projects
None yet
3 participants
Contributor

anntzer commented Jun 9, 2016

  1. Remove unused entries from STEP_LOOKUP_MAP (plt.step already
    normalizes drawstyles to step-* format).
  2. Clarify the error for invalid drawstyles.
  3. Fix handling of step-* drawstyles by the Qt options editor.

mdboom added the needs_review label Jun 9, 2016

Owner

tacaswell commented Jun 10, 2016

https://travis-ci.org/matplotlib/matplotlib/jobs/136321898#L1084

Apparently that feature got in with out enough tests.

tacaswell added this to the 2.1 (next point release) milestone Jun 10, 2016

@anntzer anntzer Cleanup for drawstyles.
1. Remove unused entries from `STEP_LOOKUP_MAP` (`plt.step` already
normalizes drawstyles to `steps-*` format, make `fill_between` and
`fill_betweenx` do the same).

2. Clarify the error for invalid drawstyles.

3. Fix handling of `steps-*` drawstyles by the Qt options editor.
b45fdde
Contributor

anntzer commented Jun 10, 2016

Fixed. I chose to normalize the input of fill_between to steps-*.

@tacaswell tacaswell merged commit c489bff into matplotlib:master Jun 19, 2016

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.002%) to 69.608%
Details

tacaswell removed the needs_review label Jun 19, 2016

anntzer deleted the anntzer:cleanup-drawstyles branch Jun 20, 2016

Contributor

anntzer commented Jul 27, 2016

Actually, can we backport this to 2.0? Up to 1.5.1, opening the figure options editor after making a plot of drawstyle "steps-post" (for example) would print an error and mangle the drawstyle, which is bad, but still much better than the situation in 2.0b3 where an exception in thrown in the event loop (... and by default, PyQt5 aborts the program in such a case).

Owner

tacaswell commented Jul 28, 2016

Does the whole PR need to go back, or just the changes to figureoptions?

Contributor

anntzer commented Jul 28, 2016

I think the latter should be enough.

Contributor

anntzer commented Sep 12, 2016

@tacaswell should I open a new PR with just the changes to figureoptions? Will that lead to merge conflicts when merging 2.0 back into master (if we ever do that?).
TBH I think the PR is small enough that splitting it is a bit overkill but either way works for me.
(Again, note that the change to figureoptions removes a possibility for a fatal exception in the Qt5 backend.)

Owner

tacaswell commented Sep 12, 2016

Can you do a PR with just the figureoptions change?

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