8000 Allow zero sized ticks by scopatz · Pull Request #7959 · matplotlib/matplotlib · GitHub
[go: up one dir, main page]
Skip to content

Allow zero sized ticks#7959

Merged
dstansby merged 3 commits intomatplotlib:masterfrom
scopatz:size0tick
Jan 29, 2017
Merged

Allow zero sized ticks#7959
dstansby merged 3 commits intomatplotlib:masterfrom
scopatz:size0tick

Conversation

@scopatz
Copy link
Contributor
@scopatz scopatz commented Jan 27, 2017

This addresses an issue with savefig() when there are no ticks present. In axis.py,

    def get_tick_space(self):
        ends = self.axes.transAxes.transform([[0, 0], [1, 0]])
        length = ((ends[1][0] - ends[0][0]) / self.axes.figure.dpi) * 72.0
        tick = self._get_tick(True)
        # There is a heuristic here that the aspect ratio of tick text
        # is no more than 3:1
        size = tick.label1.get_size() * 3
       return int(np.floor(length / size))

if size == 0, then the this will be effectively, int(np.floor(np.infty)) which fails on integer conversion with,

>>> int(np.floor(np.infty))
OverflowError: cannot convert float infinity to integer

Since this is directly used in ticker.py in _raw_ticks.py in a minimization, it is sufficient to just return a 'big' number. The full traceback we are seeing in the xonsh test suite is,

__________________________________ test_mpl_preserve_image_tight ___________________________________

    def test_mpl_preserve_image_tight():
        """Make sure that the figure preserves height settings"""
        f = create_figure()
        exp = mplhooks.figure_to_rgb_array(f)
        width, height = f.canvas.get_width_height()
>       s = mplhooks.figure_to_tight_array(f, 0.5*width, 0.5*height, True)

test_mpl.py:94: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
../xontrib/mplhooks.py:105: in figure_to_tight_array
    array = figure_to_rgb_array(fig, shape=(height, width, 4))
../xontrib/mplhooks.py:48: in figure_to_rgb_array
    array = np.frombuffer(_get_buffer(fig, dpi=fig.dpi, format='raw').read(), dtype='uint8')
../xontrib/mplhooks.py:24: in _get_buffer
    fig.savefig(b, **kwargs)
../../miniconda/lib/python3.5/site-packages/matplotlib/figure.py:1572: in savefig
    self.canvas.print_figure(*args, **kwargs)
../../miniconda/lib/python3.5/site-packages/matplotlib/backends/backend_qt5agg.py:222: in print_figure
    FigureCanvasAgg.print_figure(self, *args, **kwargs)
../../miniconda/lib/python3.5/site-packages/matplotlib/backend_bases.py:2244: in print_figure
    **kwargs)
../../miniconda/lib/python3.5/site-packages/matplotlib/backends/backend_agg.py:526: in print_raw
    FigureCanvasAgg.draw(self)
../../miniconda/lib/python3.5/site-packages/matplotlib/backends/backend_agg.py:464: in draw
    self.figure.draw(self.renderer)
../../miniconda/lib/python3.5/site-packages/matplotlib/artist.py:63: in draw_wrapper
    draw(artist, renderer, *args, **kwargs)
../../miniconda/lib/python3.5/site-packages/matplotlib/figure.py:1143: in draw
    renderer, self, dsu, self.suppressComposite)
../../miniconda/lib/python3.5/site-packages/matplotlib/image.py:139: in _draw_list_compositing_images
    a.draw(renderer)
../../miniconda/lib/python3.5/site-packages/matplotlib/artist.py:63: in draw_wrapper
    draw(artist, renderer, *args, **kwargs)
../../miniconda/lib/python3.5/site-packages/matplotlib/axes/_base.py:2409: in draw
    mimage._draw_list_compositing_images(renderer, self, dsu)
../../miniconda/lib/python3.5/site-packages/matplotlib/image.py:139: in _draw_list_compositing_images
    a.draw(renderer)
../../miniconda/lib/python3.5/site-packages/matplotlib/artist.py:63: in draw_wrapper
    draw(artist, renderer, *args, **kwargs)
../../miniconda/lib/python3.5/site-packages/matplotlib/axis.py:1136: in draw
    ticks_to_draw = self._update_ticks(renderer)
../../miniconda/lib/python3.5/site-packages/matplotlib/axis.py:969: in _update_ticks
    tick_tups = [t for t in self.iter_ticks()]
../../miniconda/lib/python3.5/site-packages/matplotlib/axis.py:969: in <listcomp>
    tick_tups = [t for t in self.iter_ticks()]
../../miniconda/lib/python3.5/site-packages/matplotlib/axis.py:912: in iter_ticks
    majorLocs = self.major.locator()
../../miniconda/lib/python3.5/site-packages/matplotlib/ticker.py:1794: in __call__
    return self.tick_values(vmin, vmax)
../../miniconda/lib/python3.5/site-packages/matplotlib/ticker.py:1802: in tick_values
    locs = self._raw_ticks(vmin, vmax)
../../miniconda/lib/python3.5/site-packages/matplotlib/ticker.py:1744: in _raw_ticks
    nbins = max(min(self.axis.get_tick_space(), 9),
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <matplotlib.axis.XAxis object at 0x7fe9effe7390>

    def get_tick_space(self):
        ends = self.axes.transAxes.transform([[0, 0], [1, 0]])
        length = ((ends[1][0] - ends[0][0]) / self.axes.figure.dpi) * 72.0
        tick = self._get_tick(True)
        # There is a heuristic here that the aspect ratio of tick text
        # is no more than 3:1
        size = tick.label1.get_size() * 3
>       return int(np.floor(length / size))
E       OverflowError: cannot convert float infinity to integer

../../miniconda/lib/python3.5/site-packages/matplotlib/axis.py:2024: OverflowError

@scopatz scopatz mentioned this pull request Jan 27, 2017
Merged
@story645
Copy link
Member
story645 commented Jan 27, 2017

Can you please add (py.test) tests?

@scopatz
Copy link
Contributor Author
scopatz commented Jan 27, 2017

can you please point me where there are similar tests? I know nothing about testing mpl these days. thanks!

story645
story645 reviewed Jan 27, 2017
View reviewed changes
lib/matplotlib/axis.py
return int(np.floor(length / size))
if size > 0:
return int(np.floor(length / size))
else:
Copy link
Member
@story645 story645 Jan 27, 2017

Choose a reason for hiding this comment

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

doesn't need the else. Also, I'd invert this and flag the special case

if size <= 0:
    return return 2**31 - 1

Copy link
Contributor Author
@scopatz scopatz Jan 27, 2017

Choose a reason for hiding this comment

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

For what it is worth, keeping the else clause is about 10% faster. The order shouldn't matter and doesn't affect timings that I have noticed.

scopatz@artemis ~ $ def f(x):
`·.,¸,.·*¯`·.,¸,.·*     if x:
`·.,¸,.·*¯`·.,¸,.·*         return 42
`·.,¸,.·*¯`·.,¸,.·*     else:
`·.,¸,.·*¯`·.,¸,.·*         return 0
`·.,¸,.·*¯`·.,¸,.·*     
`·.,¸,.·*¯`·.,¸,.·* 
scopatz@artemis ~ $ def g(x):
`·.,¸,.·*¯`·.,¸,.·*     if x:
`·.,¸,.·*¯`·.,¸,.·*         return 42
`·.,¸,.·*¯`·.,¸,.·*     return 0
`·.,¸,.·*¯`·.,¸,.·* 
scopatz@artemis ~ $ timeit! f(True)
10000000 loops, best of 3: 57.6 ns per loop
scopatz@artemis ~ $ timeit! g(True)
10000000 loops, best of 3: 63.6 ns per loop
scopatz@artemis ~ $ timeit! f(False)
10000000 loops, best of 3: 58 ns per loop
scopatz@artemis ~ $ timeit! g(False)
10000000 loops, best of 3: 64 ns per loop
scopatz@artemis ~ $ 1 - (58.0/64)
0.09375

Copy link
Member
@story645 story645 Jan 27, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I know order doesn't matter, I just think from a clarity/maintance point of view the special case should be in the if.

Though now I'm wondering if there's a way to modify int(np.floor(length/size)) such that it'll catch the size<=0 case

Copy link
Contributor Author
@scopatz scopatz Jan 27, 2017

Choose a reason for hiding this comment

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

It is probably a bit of apples and oranges since I find the way I wrote it clearest. I looked around for something in numpy that would cast infinities to integers better, but I didn't come up with anything.

Copy link
Member
@story645 story645 Jan 27, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

shrugs

Copy link
Member
@NelleV NelleV Jan 27, 2017

Choose a reason for hiding this comment

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

I also find the if else statement clearer.

Copy link
Member
@story645 story645 Jan 27, 2017

Choose a reason for hiding this comment

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

shrugs wasn't gonna put up a fight

lib/matplotlib/axis.py
return int(np.floor(length / size))
if size > 0:
return int(np.floor(length / size))
else:
Copy link
Member
@story645 story645 Jan 27, 2017

Choose a reason for hiding this comment

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

same, don't need else

@tacaswell tacaswell added this to the 2.0.1 (next bug fix release) milestone Jan 27, 2017
@tacaswell tacaswell added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Jan 27, 2017
@story645
Copy link
Member
story645 commented Jan 27, 2017 •
edited
Loading

:/ ugh, unfortunately there are no tests for axis, so at least as a first pass maybe test that savefig now behaves as expected. Savefig tests are in test_backend_ps.py but the fix should maybe be in base if it applies everywhere.

@NelleV
Copy link
Member
NelleV commented Jan 27, 2017

Hi @scopatz
The patch looks good.
As for testing, I think a simple test that shows it doesn't crash would be ok, so just add the code you used to illustrate the bug to test_axes.py in a test function, and it should be fine. No need to worry about pytest in this particular cas.

@scopatz
Copy link
Contributor Author
scopatz commented Jan 27, 2017

Thanks @NelleV! I have added a minimal test that reproduces the issue. Let me know if this needs anything else.

story645
story645 reviewed Jan 27, 2017
View reviewed changes
lib/matplotlib/tests/test_axes.py Outdated


@cleanup
def test_size0_ticks():
Copy link
Member
@story645 story645 Jan 27, 2017

Choose a reason for hiding this comment

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

can you name this test_tick_space_size_0 so it refers to the function being tested?

Copy link
Contributor Author
@scopatz scopatz Jan 27, 2017

Choose a reason for hiding this comment

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

done.

Copy link
Member
@story645 story645 Jan 27, 2017

Choose a reason for hiding this comment

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

:) LGTM, just waiting for the tests to clear

NelleV
NelleV approved these changes Jan 27, 2017
View reviewed changes
Copy link
Member
@NelleV NelleV left a comment

Choose a reason for hiding this comment

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

Thanks @scopatz !

@NelleV NelleV changed the title Allow zero sized ticks [MRG+1] Allow zero sized ticks Jan 27, 2017
@story645 story645 closed this Jan 27, 2017
@story645 story645 reopened this Jan 27, 2017
@dstansby
Copy link
Member
dstansby commented Jan 29, 2017

👍 Looks good, thanks @scopatz

@dstansby dstansby merged commit 0799250 into matplotlib:master Jan 29, 2017
@dstansby dstansby changed the title [MRG+1] Allow zero sized ticks Allow zero sized ticks Jan 29, 2017
@scopatz
Copy link
Contributor Author
scopatz commented Jan 29, 2017

Thanks All!

@scopatz scopatz deleted the size0tick branch January 29, 2017 20:16
NelleV pushed a commit that referenced this pull request Jan 29, 2017
@dstansby @NelleV
Merge pull request #7959 from scopatz/size0tick
729d219 
Allow zero sized ticks
@NelleV
Copy link
Member
NelleV commented Jan 29, 2017

backported to v2.0.x as 729d219

@dstansby
Copy link
Member
dstansby commented Jan 29, 2017

Woops, sorry for missing the backport and thanks for picking it up.

@tacaswell tacaswell mentioned this pull request Feb 23, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@story645 story645 story645 left review comments

@NelleV NelleV NelleV approved these changes

@dstansby dstansby dstansby approved these changes

Assignees

No one assigned

Labels

Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions.

Projects

None yet

Milestone

v2.0.1

Development

Successfully merging this pull request may close these issues.

5 participants

@scopatz @story645 @NelleV @dstansby @tacaswell
0