FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Fix MultipleLocator proposed by @anntzer by rougier · Pull Request #7123 · matplotlib/matplotlib · GitHub

  1. matplotlib
  2. matplotlib

Repository navigation

Fix MultipleLocator proposed by @anntzer - #7123

Closed
rougier wants to merge 1 commit into
matplotlib:masterfrom
rougier:fix_ticker
ConversationCommits (1)ChecksFiles changed
Closed

Fix MultipleLocator proposed by @anntzer#7123
rougier wants to merge 1 commit into
matplotlib:masterfrom
rougier:fix_ticker

Conversation

rougier commented Sep 17, 2016

Copy link
Copy Markdown
Member

This is the fix proposed by @anntzer in #7122 related to a bug in MultipleLocator ticker.

rougier mentioned this pull request Sep 17, 2016
Merged
NelleV reviewed Sep 17, 2016
View reviewed changes
Comment thread lib/matplotlib/ticker.py
locs = vmin - base + np.arange(n + 3) * base
nmin = round(self._base.ge(vmin) / base)
nmax = round(self._base.le(vmax) / base)
locs = np.arange(nmin, nmax + 1) * base

NelleV Sep 17, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Why can't this code be simplified to: `np.arange(vmin, vmax + base, base)? Floating point errors?

NelleV Sep 17, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Sorry… np.arange(self._base.ge(vmin), vmax + base, base) would be the correct solution.

anntzer Sep 19, 2016 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Your proposal can include a value that's above vmax (eg. vmin=.5, vmax=2.5, base=1 will return [1, 2, 3]). I guess another solution that's safe against floating point errors is
np.arange(self._base.ge(vmin), self._base.le(vmax) + base / 2, base)
(the + base / 2 should take care of any fpe).

NelleV Sep 19, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Yep: I am just concern about the tests that don't make sense to me.

anntzer Sep 19, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I had a quick look and am pretty sure that the test values are wrong (i.e. they were picked to match the previous algorithm, which returns out of bounds values; they should be changed to match the new algorithm).

NelleV commented Sep 19, 2016

Copy link
Copy Markdown
Member

I have done a couple of tests locally, and I have to admit that the current tests seems obscure…
@pelson you wrote this test. Can you comment on this? Some ticks are outside of the bounds provided by the function: it doesn't seem the correct behavior to me.

anntzer mentioned this pull request Sep 19, 2016
Closed
tacaswell added this to the 2.0.1 (next bug fix release) milestone Sep 19, 2016

tacaswell commented Sep 22, 2016

Copy link
Copy Markdown
Member

I think that this ties back to the other issue with assuming all of the ticks are in view limits (#6647) and what layer of the stack is responsible for the final filtering.

tacaswell commented Sep 22, 2016

Copy link
Copy Markdown
Member

When this is cleared up please clean up the figure added in #7084

QuLogic modified the milestones: 2.0.1 (next bug fix release), 2.0.2 (next bug fix release) May 3, 2017
tacaswell modified the milestones: 2.1.1 (next bug fix release), 2.2 (next feature release) Oct 9, 2017

jklymak commented Jul 14, 2020

Copy link
Copy Markdown
Member

This is pretty stale, so I'm assuming this change isn't urgently required by anyone, but feel free to reopen if wanted...

jklymak closed this Jul 14, 2020
QuLogic added the stale label Jul 14, 2020
story645 removed this from the future releases milestone Oct 6, 2022
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

NelleV NelleV left review comments
anntzer anntzer left review comments

Assignees

No one assigned

Labels

status: needs revision

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

Back | FazBrowse Home | New Git URL