Opened 8 years ago
Closed 8 years ago
#18612 closed defect (fixed)
Yaxis labels on matrix_plot are reversed.
Reported by:  David Einstein  Owned by:  

Priority:  blocker  Milestone:  sage6.8 
Component:  graphics  Keywords:  
Cc:  KarlDieter Crisman, Steven Trogdon, William Stein  Merged in:  
Authors:  David Einstein  Reviewers:  KarlDieter Crisman 
Report Upstream:  N/A  Work issues:  
Branch:  008ece0 (Commits, GitHub, GitLab)  Commit:  008ece0d575e257071ea2ac3daf014319623bf1b 
Dependencies:  Stopgaps: 
Description
This is a regression caused by the partial fix in #18463.
matrix_plot(identity_matrix(5))
has the yaxis labels ascending, while they should be descending.
Attachments (1)
Change History (18)
Changed 8 years ago by
Attachment:  tmp_ST550r.png added 

comment:1 Changed 8 years ago by
Cc:  William Stein added 

Priority:  major → blocker 
comment:2 Changed 8 years ago by
sage: p = 13; matrix_plot(matrix(p1,[mod(a,p)^b for a in range(1,p) for b in srange(p)]),cmap='jet') Launched png viewer for Graphics object consisting of 1 graphics primitive
Looks great, but yaxis labels reversed (known bug from #18463).
sage: p = 13; matrix_plot(matrix(p1,[mod(a,p)^b for a in range(1,p) for b in srange(p)]),cmap='jet', origin='lower') Launched png viewer for Graphics object consisting of 1 graphics primitive
Labels correct, but image upsidedown.
Could we just ("just") use pyplot.yticks to get the vertical ticks and then immediately reverse their order? Or something like
sage: m.gca().get_yticks() array([ 2., 0., 2., 4., 6., 8., 10., 12.]) sage: m.gca().set_yticks(m.gca().get_yticks()[::1])
I can't get this to quite work because I don't have the tkagg enabled for my Sage matplotlib, but inside of Sage something doing this should (hopefully) work.
http://stackoverflow.com/questions/9382664/pythonmatplotlibimshowcustomtickmarks
http://matplotlib.org/api/axes_api.html#matplotlib.axes.Axes.get_yticks
comment:3 Changed 8 years ago by
I think that it is more complicated than that.
I know next to nothing about the Sage graphics system, please feel free to clarify any blatant falsehoods.
The MatrixPlot? object is a GraphicsPrimitive? and is responsible for drawing the image. The axes are drawn by the containing Graphics object which contains the MatrixPlot?. There may be a member of Graphics that allows us to relabel the axes that we could call in matrix_plot
, but I do not know what it would be.
The more I look at this, I think that the best short term solution would be to back out #18463 and put an abs
in _limit_output_aspect_ratio
. This is possibly wrong in terms of composed plots, but things would work correctly for the use cases that we have now. It would not be incorrect in the case that ymax>ymin, just somewhat wasteful.
Does there exist high level documentation of the Graphics subsytem somewhere? Something which describes how sums of plots are supposed to work, how to draw different axes for different plots in a single Graphics object would be nice.
comment:4 Changed 8 years ago by
Cc:  Steven Trogdon added 

comment:5 Changed 8 years ago by
Hmm, you may be right. Yes, composed plots will probably nearly always be wrong, but I'm not sure anyone anticipated combining "regular" plots with matrix plots or indeed even combining two matrix plots.
Given the many reports, we need a fix for #18463, and I would strongly prefer one that is less elegant but preserves current functionality. As Volker says elsewhere, the graphics code is convoluted enough, so this will not do much damage there.
comment:6 Changed 8 years ago by
Branch:  → u/deinst/y_axis_labels_on_matrix_plot_are_reversed_ 

comment:7 Changed 8 years ago by
Authors:  → David Einstein 

Commit:  → 05875d563029eb7231d2cae4b434b99e4742fbbf 
Status:  new → needs_review 
Ok, I am at Juliacon and don't have a bunch of time (not enough to run the long tests). This is the proposed fix. It appears to work, but is at least philosophically ugly.
comment:8 Changed 8 years ago by
I think you may have put the wrong branch? It says "already merged". Thanks very much though, I will check it out today or tomorrow.
comment:9 Changed 8 years ago by
Commit:  05875d563029eb7231d2cae4b434b99e4742fbbf → 008ece0d575e257071ea2ac3daf014319623bf1b 

Branch pushed to git repo; I updated commit sha1. New commits:
008ece0  add abs to scaling of aspect ratio

comment:11 followup: 12 Changed 8 years ago by
William, if you see this  would this solution suffice for SMC purposes as well?
Regarding ugly, comment:4:ticket:18463 already points out that problem. I just think that we haven't really ever talked about adding matrix plots to each other or anything else in documentation, and it's best to fix that elsewhere  hopefully in a way that also fixes this problem 'correctly'.
Also, for my or other testing reference, do
sage: i = identity_matrix(10,sparse=True) sage: matrix_plot(i) sage: matrix_plot(i, aspect_ratio='automatic')
as an example that David mentioned in the previous ticket.
comment:12 Changed 8 years ago by
sage: i = identity_matrix(10,sparse=True) sage: matrix_plot(i)
Oh, I remember now  that is just a difference in how we plotted sparse matrices, not related to all this stuff.
comment:13 Changed 8 years ago by
Reviewers:  → KarlDieter Crisman 

I'm happy with this, ugly or not.
Can you think of ANY way for us to doctest the graphic itself (perhaps the bounding box or other attribute) to ensure this doesn't happen again?
comment:14 Changed 8 years ago by
Oh wait, I forgot to make sure that this still makes things work in general, since it actually changes all graphics, not just matrix plot. But I'm happy on the matrix plot side; if anyone checks that this works for normal graphics (including ones with ymin>ymax
etc.) then we are good to go.
Update: they seem to in spot checks. Running doctests.
comment:16 Changed 8 years ago by
Status:  needs_review → positive_review 

Okay, I can't find any problems here in spot checking other graphics types and using those keywords, as expected.
comment:17 Changed 8 years ago by
Branch:  u/deinst/y_axis_labels_on_matrix_plot_are_reversed_ → 008ece0d575e257071ea2ac3daf014319623bf1b 

Resolution:  → fixed 
Status:  positive_review → closed 
Incorrect image