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

Avoid dividing by zero in AutoMinorLocator (fixes #8804) #9465

Merged

Conversation

afvincent
Copy link
Contributor

@afvincent afvincent commented Oct 18, 2017

PR Summary

Get rid of 2 DivisionByZero warnings by performing a proper early return, which prevents the case noticed by @ffteja in #8804 from happening.

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

@afvincent afvincent modified the milestones: v2.2, v2.1.1 Oct 18, 2017
@@ -2545,7 +2545,7 @@ def __call__(self):
if vmin > vmax:
vmin, vmax = vmax, vmin

if len(majorlocs) > 0:
if len(majorlocs) >= 2:
Copy link
Contributor

Choose a reason for hiding this comment

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

can you just do an early return [] around line 2522 instead of having to handle this case throughout the function?

@afvincent afvincent force-pushed the fix_div_by_zero_autominorlocator_issue_8804 branch from f51fa53 to 44677ff Compare October 18, 2017 18:07
@afvincent
Copy link
Contributor Author

@anntzer An early return is now performed. I also extended a bit the dedicated test.

@@ -2524,7 +2524,7 @@ def __call__(self):
# TODO: Figure out a way to still be able to display minor
# ticks without two major ticks visible. For now, just display
# no ticks at all.
majorstep = 0
return []

if self.ndivs is None:
if majorstep == 0:
Copy link
Contributor

Choose a reason for hiding this comment

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

i think this can go away too now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed... I was a bit too quick.

@afvincent afvincent force-pushed the fix_div_by_zero_autominorlocator_issue_8804 branch from 44677ff to 3418d96 Compare October 18, 2017 19:17
@afvincent
Copy link
Contributor Author

Address @anntzer's last comment (about removing now unnecessary code) and rebased. (The new test was kept unmodified and is still passing)

Copy link
Contributor

@anntzer anntzer 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 tests

@NelleV NelleV merged commit 473e25c into matplotlib:master Oct 20, 2017
@NelleV
Copy link
Member

NelleV commented Oct 20, 2017

Thanks @afvincent !

tacaswell added a commit that referenced this pull request Oct 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants