Skip to content

BUG: correct the scaling in the floating-point slop test. #11591

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.

Sign up for GitHub

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

Jump to bottom
Merged
tacaswell merged 3 commits into from
Aug 22, 2018
Merged

BUG: correct the scaling in the floating-point slop test. #11591

tacaswell merged 3 commits into from
Aug 22, 2018

Conversation

@efiring
Copy link
Member

@efiring efiring commented Jul 6, 2018 •
edited
Loading

Closes #11587.

PR Summary

PR Checklist

  • Has Pytest style unit tests
  • Code is PEP 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

jklymak
jklymak previously approved these changes Jul 6, 2018
View reviewed changes
Copy link
Member

@jklymak jklymak left a comment

Choose a reason for hiding this comment

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

Looks right to me. Does this merit a test?

@jklymak
Copy link
Member

jklymak commented Jul 6, 2018

Ooops, though this fails test_axes.py::test_autoscale_tiny_range

@efiring
Copy link
Member Author

efiring commented Jul 7, 2018

That blows the lid off the worm can. Working on it.

@tacaswell tacaswell added this to the v2.2.3 milestone Jul 7, 2018
@efiring
Copy link
Member Author

efiring commented Jul 10, 2018

I think this is getting close to a reasonable solution. Base is confusing and broken (and we use only part of it), so I would like to deprecate it in favor of the private _Edge_integer class, thereby reducing the public API; this is really implementation detail that we should be free to change. _Edge_integer probably can be improved (and maybe someone has a better name), but it is at least doing the right general thing in adapting its closeto to the ratio between the offset and the step, which Base does not do.

jklymak
jklymak reviewed Jul 10, 2018
View reviewed changes
lib/matplotlib/ticker.py
return scale, offset


class _Edge_integer:
Copy link
Member

@jklymak jklymak Jul 10, 2018

Choose a reason for hiding this comment

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

Needs some documentation!

jklymak
jklymak reviewed Jul 10, 2018
View reviewed changes
lib/matplotlib/ticker.py
high = np.round(Base(step).ge(_vmax - best_vmin) / step)
ticks = np.arange(low, high + 1) * step + best_vmin + offset
nticks = ((ticks <= vmax) & (ticks >= vmin)).sum()
edge = _Edge_integer(step, offset)
Copy link
Member

@jklymak jklymak Jul 10, 2018

Choose a reason for hiding this comment

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

Similarly, this whole block could use a couple lines of comment. I looked at this the other day, and worked my way through it, but on its face it looks more complicated than whats needed.

@efiring efiring force-pushed the MaxNLocator_vmax_bug branch from b688e2c to 9d33cb5 Compare July 11, 2018 01:43
efiring added 2 commits July 10, 2018 21:06
@efiring
BUG: replace buggy ticker.Base with _Edge_integer.
2b40260 
Closes matplotlib#11587.

Tests were working by accident despite bugs in ticker.Base, but
the bugs caused problems in some applications of MaxNLocator,
as in matplotlib#11587.  Since some user code may be relying on the buggy
behavior, ticker.Base is left in place but deprecated, and its
intended functionality is provided by a new private class,
_Edge_integer.  MaxNLocator and MultipleLocator are modified
to use the new class, with minor cleanups and an additional
bug fix along the way.

Two new relevant test cases are added to TestMaxNLocator.
@efiring
Deprecate ticker.Base.
74c2e7c
@efiring efiring force-pushed the MaxNLocator_vmax_bug branch from 245fe2c to 74c2e7c Compare July 11, 2018 07:17
@efiring
Copy link
Member Author

efiring commented Jul 11, 2018

I think this is ready to go now, with the possible exception of a next_api_changes note for the deprecation of ticker.Base and ticker.closeto.

@jklymak jklymak dismissed their stale review July 11, 2018 22:45

stale

jklymak
jklymak approved these changes Jul 11, 2018
View reviewed changes
Copy link
Member

@jklymak jklymak left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks!

@tacaswell
Copy link
Member

tacaswell commented Jul 15, 2018

@efiring re-milestoned as 3.1 based on your deprecation tags.

@tacaswell tacaswell modified the milestones: v2.2.3, v3.1 Jul 15, 2018
@efiring
Copy link
Member Author

efiring commented Jul 15, 2018

I would be happy to move the tags up to 3.0 if you are interested in squeezing this into 3.0. Leaving it as 3.1 is OK, too.

@jklymak jklymak modified the milestones: v3.1, v3.0 Aug 20, 2018
tacaswell
tacaswell approved these changes Aug 20, 2018
View reviewed changes
Copy link
Member

@tacaswell tacaswell left a comment

Choose a reason for hiding this comment

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

Conditional on fixing the deprecation labels.

@efiring
Move deprecation back to 3.0.
4628ceb 
Also, clarify a downward-counting for-loop.
@tacaswell tacaswell merged commit 1834e0e into matplotlib:master Aug 22, 2018
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Aug 22, 2018
@tacaswell
Backport PR matplotlib#11591: BUG: correct the scaling in the floatin…
1ecc985 
…g-point slop test.
@meeseeksmachine meeseeksmachine mentioned this pull request Aug 22, 2018
Merged
tacaswell added a commit that referenced this pull request Aug 23, 2018
@tacaswell
Merge pull request #11915 from meeseeksmachine/auto-backport-of-pr-11…
5a4b8d8 
…591-on-v3.0.x

Backport PR #11591 on branch v3.0.x
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tacaswell tacaswell tacaswell approved these changes

@jklymak jklymak jklymak approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

v3.0.0

Development

Successfully merging this pull request may close these issues.

Missing filled contours when using contourf

4 participants

@efiring @jklymak @tacaswell @QuLogic