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

FIX: check if contour level in format dictionary, or return a default #9743

Merged
merged 2 commits into from Nov 12, 2017

Conversation

Projects
None yet
4 participants
@jklymak
Copy link
Contributor

commented Nov 10, 2017

PR Summary

As per #9742 the user could pass a format dictionary to clabel that doesn't contain a contour level. Normally this would be a too-bad-so-sad sort of thing, except if there are not contours we change the value of the contour ( 6ac197f ).

This fix is maybe suboptimal in that the user may wonder why a contour label sneaks in with a different format. However, I doubt too many users would specify the contour formats but not the levels, so this really only handles the edgecase in #9742.

PR Checklist

  • Has Pytest style unit tests
  • Code is PEP 8 compliant
@@ -328,7 +328,10 @@ def get_text(self, lev, fmt):
return lev
else:
if isinstance(fmt, dict):
return fmt[lev]

This comment has been minimized.

Copy link
@tacaswell

tacaswell Nov 10, 2017

Member

Doing this as fmt.get(lev, '%1.3f') is a bit terser.

@tacaswell tacaswell added this to the v2.1.1 milestone Nov 10, 2017

@jklymak jklymak force-pushed the jklymak:fixclabel branch from fff6932 to 1c67f1c Nov 10, 2017

@jklymak jklymak force-pushed the jklymak:fixclabel branch from 1c67f1c to 95fd554 Nov 10, 2017

@efiring
Copy link
Member

left a comment

Looks OK apart from the question about the test.

with pytest.warns(UserWarning) as record:
cs = ax.contour(x, x, z, levels=[1.])
ax.clabel(cs, fmt=fmt)
assert len(record) == 1

This comment has been minimized.

Copy link
@efiring

efiring Nov 11, 2017

Member

Shouldn't this be inside the with block?

This comment has been minimized.

Copy link
@jklymak

jklymak Nov 12, 2017

Author Contributor

I just copied the previous test. Seems to work but happy to change if wrong

This comment has been minimized.

Copy link
@efiring

efiring Nov 12, 2017

Member

OK, I see now where this style is shown in the pytest documentation, so no problem.

@dstansby dstansby merged commit 7252158 into matplotlib:master Nov 12, 2017

7 checks passed

ci/circleci: docs-python27 Your tests passed on CircleCI!
Details
ci/circleci: docs-python35 Your tests passed on CircleCI!
Details
codecov/patch 100% of diff hit (target 50%)
Details
codecov/project/library 61.61% (target 50%)
Details
codecov/project/tests Absolute coverage decreased by -0.02% but relative coverage increased by +1.19% compared to d8154cb
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
lgtm analysis: Python No alert changes
Details

meeseeksdev bot pushed a commit that referenced this pull request Nov 12, 2017

NelleV added a commit that referenced this pull request Nov 12, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.