#13625 closed defect (fixed)
matrix_plot and title don't mix well (yet)
Reported by:  KarlDieter Crisman  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 10 years ago by
Attachment:  Lenawrongtitle.png added 

comment:1 Changed 10 years ago by
Description:  modified (diff) 

comment:2 Changed 10 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 10 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 10 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 10 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 10 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 10 years ago by
Report Upstream:  N/A → 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 10 years ago by
Description:  modified (diff) 

Report Upstream:  Not yet reported upstream; Will do shortly. → Reported upstream. No feedback yet. 
OK. This is now reported upstream at 1415.
comment:9 Changed 10 years ago by
Report Upstream:  Reported upstream. No feedback yet. → 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 10 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 10 years ago by
Description:  modified (diff) 

Status:  new → needs_review 
Added patches, which must be applied in the specified order.
comment:12 Changed 10 years ago by
Authors:  → 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 10 years ago by
Thanks. I forgot about the origin
keyword. You did mention this before.
comment:14 Changed 10 years ago by
patchbot apply trac_13625new_option_title_pos.patch trac_13625add_title_pos_to_matrix_plot.patch
comment:15 Changed 10 years ago by
Reviewers:  → KarlDieter Crisman 

Status:  needs_review → 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.
Changed 10 years ago by
Attachment:  trac_13625add_title_pos_to_matrix_plot.patch added 

Apply to devel/sage
comment:18 Changed 10 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 10 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 10 years ago by
Description:  modified (diff) 

comment:21 Changed 10 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 10 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 10 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.
Changed 10 years ago by
Attachment:  trac_13625add_title_pos_to_matrix_plotreviewed.patch added 

comment:25 Changed 10 years ago by
Description:  modified (diff) 

Status:  needs_review → 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 10 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
Changed 10 years ago by
Attachment:  trac_13625new_option_title_pos.patch added 

Apply to devel/sage
comment:27 Changed 10 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 10 years ago by
Milestone:  sage5.5 → sage5.6 

comment:29 Changed 10 years ago by
Merged in:  → sage5.6.beta1 

Resolution:  → fixed 
Status:  positive_review → closed 
comment:30 Changed 4 years ago by
Just as FYI, https://github.com/matplotlib/matplotlib/issues/1415 might affect this.
What happens currently  bad