Opened 7 years ago

Closed 7 years ago

Last modified 19 months ago

#13625 closed defect (fixed)

matrix_plot and title don't mix well (yet)

Reported by: kcrisman Owned by: jason, was
Priority: major Milestone: sage-5.6
Component: graphics Keywords:
Cc: Merged in: sage-5.6.beta1
Authors: Punarbasu Purkayastha Reviewers: Karl-Dieter Crisman
Report Upstream: Reported upstream. Developers acknowledge bug. Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by kcrisman)

See this ask.sagemath.org question, where elcojon gives this example.

piclabels = ('width [px]','height [px]')
import scipy 
data = scipy.lena()
matrix_plot(data,axes_labels=piclabels, title='Lena image')

What happens currently - bad

(Note that this doesn't work literally in Sage 5.4.1 - the new Scipy in #13541 seems to have moved this to scipy.misc.lena().)

This is fallout from the great new functionality in #10512. The axes_labels are irrelevant to whether this happens.


Reported upstream at 1415.


Patch to workaround this issue while upstream decides how best to fix this bug. Apply in this order to devel/sage:

  1. trac_13625-new_option_title_pos.patch
  2. trac_13625-add_title_pos_to_matrix_plot-reviewed.patch

Attachments (4)

Lena-wrongtitle.png (290.8 KB) - added by kcrisman 7 years ago.
What happens currently - bad
trac_13625-add_title_pos_to_matrix_plot.patch (2.0 KB) - added by ppurka 7 years ago.
Apply to devel/sage
trac_13625-add_title_pos_to_matrix_plot-reviewed.patch (2.8 KB) - added by kcrisman 7 years ago.
trac_13625-new_option_title_pos.patch (5.4 KB) - added by ppurka 7 years ago.
Apply to devel/sage

Download all attachments as: .zip

Change History (35)

Changed 7 years ago by kcrisman

What happens currently - bad

comment:1 Changed 7 years ago by kcrisman

  • Description modified (diff)

comment:2 Changed 7 years ago by ppurka

Unless I am mistaken, this has to be a bug in matplotlib. We don't touch title positions when there is a frame.

comment:3 Changed 7 years ago by kcrisman

That's my point; we do touch them when we have axes but no frames, so that there isn't an overlap (see the discussion at #10512. So maybe we should do so here as well. Perhaps those are both bugs in mpl, in which case maybe it should be reported upstream, but I don't have enough mpl-fu to figure out a native mpl version of this problem quickly. I think it's us, because in the code for

sage: sage.plot.matrix_plot.MatrixPlot._render_on_subplot??
<skip stuff>
image=subplot.imshow(self.xy_data_array, **opts)
<skip irrelevant stuff>
subplot.xaxis.tick_top()

we presumably add the title before the ticks and labels. So it's already placed the title early. Maybe we should do all opts except title, and then do the title at the end in this particular rendering method? I'll try that quick.

comment:4 Changed 7 years ago by kcrisman

Scratch that last comment,

            opts = dict(cmap=cmap, interpolation='nearest', aspect='equal',
                      norm=norm, vmin=options['vmin'], vmax=options['vmax'],
                      origin=origin,zorder=options.get('zorder',None))

so I'll have to look a little more at where the title enters.

comment:5 Changed 7 years ago by kcrisman

  • Description modified (diff)

Indeed, the following fixes the problem (though probably introduces others!)

  • sage/plot/graphics.py

    diff --git a/sage/plot/graphics.py b/sage/plot/graphics.py
    a b  
    24602460        # free limits autoscale
    24612461        #subplot.autoscale_view(tight=True)
    24622462        if title is not None:
    2463             if (frame) or (axes_labels is None):
    2464                 subplot.set_title(title, fontsize=fontsize)
    2465             else: # frame is false axes is not None, and neither is axes_labels
    2466                 # Then, the title is moved up to avoid overlap with axes labels
    2467                 subplot.set_title(title, fontsize=fontsize, position=(0.5,1.05))
     2463            # Then, the title is moved up to avoid overlap with axes labels
     2464            subplot.set_title(title, fontsize=fontsize, position=(0.5,1.05))

comment:6 Changed 7 years ago by ppurka

It's a bug in mpl. It shouldn't be our job to move title positions around. The bug is because of tick_top() that is used.

sage: from matplotlib.figure import Figure
sage: figure = Figure()
sage: subplot = figure.add_subplot(111)
sage: import numpy as np
sage: M = np.identity(50)
sage: subplot.imshow(M)
<matplotlib.image.AxesImage object at 0x8bc0650>
sage: subplot.xaxis.tick_top()   # <--------- this introduces the bug
sage: subplot.set_title('a title')
<matplotlib.text.Text object at 0x8c54610>
sage: from matplotlib.backends.backend_agg import FigureCanvasAgg
sage: figure.set_canvas(FigureCanvasAgg(figure))
sage: figure.savefig('/tmp/a.png')
sage: !feh /tmp/a.png

EDIT: the problem with shifting title positions by ourselves is that the title will be quite a bit off vertically in normal plots. Definitely not what we desire.

Last edited 7 years ago by ppurka (previous) (diff)

comment:7 Changed 7 years ago by kcrisman

  • Report Upstream changed from N/A to Not yet reported upstream; Will do shortly.

Gotcha. I was still trying to figure out exactly how to create what you got up there...

Right, I figured that uniformly shifting the title was bad, but I was wondering if we could special-case this for now somehow while still reporting the problem. Can you report this upstream?

comment:8 Changed 7 years ago by ppurka

  • Description modified (diff)
  • Report Upstream changed from Not yet reported upstream; Will do shortly. to Reported upstream. No feedback yet.

OK. This is now reported upstream at 1415.

comment:9 Changed 7 years ago by ppurka

  • Report Upstream changed from Reported upstream. No feedback yet. to Reported upstream. Developers acknowledge bug.

I will try and see sometime if something can be hacked up in Sage. Maybe give users the extra control over where to put the plot title? Then it will have the added benefit of us not having to remember to remove our hack once mpl fixes it in their end. Something like title_pos=(x_factor, y_factor) can be used. Example would be

plot(x, title='such is life', title_pos=(0.4, 0.2))

comment:10 Changed 7 years ago by kcrisman

That seems reasonable, as long as it's automatic for matrix plots which have origin='upper'. We'd need very good documentation about what the title_pos argument means, though, since I think it's different (relative to the figure as a 1 by 1 object) than other places where things can get set, like text(), where axis_coords is not the default.

comment:11 Changed 7 years ago by ppurka

  • Description modified (diff)
  • Status changed from new to needs_review

Added patches, which must be applied in the specified order.

  1. trac_13625-new_option_title_pos.patch
  2. trac_13625-add_title_pos_to_matrix_plot.patch

comment:12 Changed 7 years ago by kcrisman

  • Authors set to Punarbasu Purkayastha

Brief glance looks good; I'm not in a position to try plots and run tests but the concept looks right for exposing this. I would point out that we only need the extra title_pos in matrix_plot when origin='upper'; it looks fine when the ticks are labeled on the bottom. You might want to experiment a little with those.

comment:13 Changed 7 years ago by ppurka

Thanks. I forgot about the origin keyword. You did mention this before.

comment:14 Changed 7 years ago by ppurka

patchbot apply trac_13625-new_option_title_pos.patch trac_13625-add_title_pos_to_matrix_plot.patch

comment:15 Changed 7 years ago by kcrisman

  • Reviewers set to Karl-Dieter Crisman
  • Status changed from needs_review to needs_work

Okay. Doctest the fixed matrix plots, with and without the keywords. People should be able to try out the difference in the live documentation.

comment:16 Changed 7 years ago by ppurka

  • Status changed from needs_work to needs_review

hopefully fixed now.

comment:17 Changed 7 years ago by ppurka

oops. sorry. Forgot about one thing.

Changed 7 years ago by ppurka

Apply to devel/sage

comment:18 Changed 7 years ago by ppurka

Ok. This is fixed now. The problem with the patch was that custom title_pos was being overridden with title_pos=(0.5,1.05). I forgot to take care of this case when I removed this keyword from @options().

comment:19 Changed 7 years ago by ppurka

patchbot failing all doctests... me tearing hair...

patchbot apply trac_13625-new_option_title_pos.patch trac_13625-add_title_pos_to_matrix_plot.patch

comment:20 Changed 7 years ago by kcrisman

  • Description modified (diff)

comment:21 Changed 7 years ago by kcrisman

Okay, looks good overall, I can do all kinds of goofy things! I like these patches, sorry for the delay.

I do think we need a little better error catching.

sage: plot(sin, -4, 4, title='Plot sin(x)', title_pos=.05)
<snip>
   2482         if title is not None:
   2483             if title_pos is not None:
-> 2484                 if len(title_pos) != 2:
   2485                     raise ValueError("`title_pos` must be a list or tuple "
   2486                                      "of two real numbers.")

TypeError: object of type 'sage.rings.real_mpfr.RealLiteral' has no len()

A subtle difference, but this should also tell the user that title_pos must be the right format.

comment:22 Changed 7 years ago by ppurka

Thanks for the review! I hadn't thought of this case. Fixed now.

Patchbot: apply trac_13625-new_option_title_pos.patch trac_13625-add_title_pos_to_matrix_plot.patch

comment:23 Changed 7 years ago by kcrisman

Should `title_pos` have back ticks in the error message?

I won't be able to get to this until next week, so no rush on the answer to that. I also have to run tests, though I don't expect any surprises.

comment:24 Changed 7 years ago by ppurka

Not sure why i used those backticks. Fixed now.

comment:25 Changed 7 years ago by kcrisman

  • Description modified (diff)
  • Status changed from needs_review to positive_review

There was also an extra space that made the LaTeX of the Cartesian product look wrong in the built doc. Otherwise, looks great, passes tests!

Patchbot, apply trac_13625-new_option_title_pos.patch and trac_13625-add_title_pos_to_matrix_plot-reviewed.patch

comment:26 Changed 7 years ago by ppurka

Oops. I didn't see that you have already fixed it! sorry. Thanks for the review. :)

Patchbot apply trac_13625-new_option_title_pos.patch and trac_13625-add_title_pos_to_matrix_plot-reviewed.patch

Changed 7 years ago by ppurka

Apply to devel/sage

comment:27 Changed 7 years ago by ppurka

reverted my change. sorry for the mess.

Patchbot apply trac_13625-new_option_title_pos.patch trac_13625-add_title_pos_to_matrix_plot-reviewed.patch

comment:28 Changed 7 years ago by jdemeyer

  • Milestone changed from sage-5.5 to sage-5.6

comment:29 Changed 7 years ago by jdemeyer

  • Merged in set to sage-5.6.beta1
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:30 Changed 19 months ago by kcrisman

comment:31 Changed 19 months ago by ppurka

Yes, may need to revisit the workaround done here.

Note: See TracTickets for help on using tickets.