#13625 closed defect (fixed)
matrix_plot and title don't mix well (yet)
Reported by:  kcrisman  Owned by:  jason, was 

Priority:  major  Milestone:  sage5.6 
Component:  graphics  Keywords:  
Cc:  Merged in:  sage5.6.beta1  
Authors:  Punarbasu Purkayastha  Reviewers:  KarlDieter Crisman 
Report Upstream:  Reported upstream. Developers acknowledge bug.  Work issues:  
Branch:  Commit:  
Dependencies:  Stopgaps: 
Description (last modified by )
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')
(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
:
Attachments (4)
Change History (35)
Changed 8 years ago by
comment:1 Changed 8 years ago by
 Description modified (diff)
comment:2 Changed 8 years ago by
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 8 years ago by
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 mplfu 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 8 years ago by
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 8 years ago by
 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 2460 2460 # free limits autoscale 2461 2461 #subplot.autoscale_view(tight=True) 2462 2462 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 8 years ago by
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.
comment:7 Changed 8 years ago by
 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 specialcase this for now somehow while still reporting the problem. Can you report this upstream?
comment:8 Changed 8 years ago by
 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 8 years ago by
 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 8 years ago by
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 8 years ago by
 Description modified (diff)
 Status changed from new to needs_review
Added patches, which must be applied in the specified order.
comment:12 Changed 8 years ago by
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 8 years ago by
Thanks. I forgot about the origin
keyword. You did mention this before.
comment:14 Changed 8 years ago by
patchbot apply trac_13625new_option_title_pos.patch trac_13625add_title_pos_to_matrix_plot.patch
comment:15 Changed 8 years ago by
 Reviewers set to KarlDieter 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 8 years ago by
 Status changed from needs_work to needs_review
hopefully fixed now.
comment:17 Changed 8 years ago by
oops. sorry. Forgot about one thing.
comment:18 Changed 8 years ago by
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 8 years ago by
patchbot failing all doctests... me tearing hair...
patchbot apply trac_13625new_option_title_pos.patch trac_13625add_title_pos_to_matrix_plot.patch
comment:20 Changed 8 years ago by
 Description modified (diff)
comment:21 Changed 8 years ago by
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 8 years ago by
Thanks for the review! I hadn't thought of this case. Fixed now.
Patchbot: apply trac_13625new_option_title_pos.patch trac_13625add_title_pos_to_matrix_plot.patch
comment:23 Changed 8 years ago by
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 8 years ago by
Not sure why i used those backticks. Fixed now.
Changed 8 years ago by
comment:25 Changed 8 years ago by
 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_13625new_option_title_pos.patch and trac_13625add_title_pos_to_matrix_plotreviewed.patch
comment:26 Changed 8 years ago by
Oops. I didn't see that you have already fixed it! sorry. Thanks for the review. :)
Patchbot apply trac_13625new_option_title_pos.patch and trac_13625add_title_pos_to_matrix_plotreviewed.patch
comment:27 Changed 8 years ago by
reverted my change. sorry for the mess.
Patchbot apply trac_13625new_option_title_pos.patch trac_13625add_title_pos_to_matrix_plotreviewed.patch
comment:28 Changed 8 years ago by
 Milestone changed from sage5.5 to sage5.6
comment:29 Changed 8 years ago by
 Merged in set to sage5.6.beta1
 Resolution set to fixed
 Status changed from positive_review to closed
comment:30 Changed 3 years ago by
Just as FYI, https://github.com/matplotlib/matplotlib/issues/1415 might affect this.
comment:31 Changed 3 years ago by
Yes, may need to revisit the workaround done here.
What happens currently  bad