Opened 4 years ago

Closed 3 years ago

#15315 closed defect (fixed)

aspect ratio ignored in matrix and density plots

Reported by: kcrisman Owned by:
Priority: major Milestone: sage-6.4
Component: graphics Keywords: matplotlib imshow density plot
Cc: ppurka, jason Merged in:
Authors: Punarbasu Purkayastha Reviewers: Karl-Dieter Crisman
Report Upstream: Reported upstream. Developers deny it's a bug. Work issues:
Branch: 2cc1beb (Commits) Commit: 2cc1beba873ee5898fadaa206cee23cd9be5455f
Dependencies: Stopgaps:

Description (last modified by ppurka)

This is due to improper use of imshow, I think. This was reported at this ask.sagemath question. See these stackoverflow questions for various approaches to setting this. Essentially, we may need to have some custom way to pass on fig size and aspect ratio to matrix and density plots (which use imshow).


Attached patch should fix aspect ratio: trac_15315-density_and_matrix_plot_aspect_ratio.patch

Attachments (1)

trac_15315-density_and_matrix_plot_aspect_ratio.patch (2.6 KB) - added by ppurka 4 years ago.

Download all attachments as: .zip

Change History (26)

comment:1 Changed 4 years ago by ppurka

Do you have an examlpe for matrix plot that ignores aspect ratio? I already have the fix to density plot, which should work for all cases - involves change in graphics.py.

comment:2 Changed 4 years ago by ppurka

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

comment:3 Changed 4 years ago by kcrisman

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

I am surprised the fix is this easy, but in retrospect it makes sense - some subplots might not obey the aspect ratio. Certainly fixes the problem in question. Obviously, needs doctests, and testing that all previous aspect ratio plots (including automatic ones) really still look correct! Good find, ppurka.

Last edited 4 years ago by kcrisman (previous) (diff)

comment:4 Changed 4 years ago by ppurka

  • Status changed from needs_work to needs_review

Well, I added a simple doctest for density plot. I have checked some of the other figures - just browse the documentation locally and evaluate the plots. It seems fine to me. :)

comment:5 follow-up: Changed 4 years ago by kcrisman

This looks great in general, except it does change density and matrix plots to having the default aspect ratio be "automatic". As a very gentle review request, could you remove the extra back tick from here? Same file. You didn't introduce it but it would be nice not to open a separate ticket for that.

As to aspect ratio, we just need to decide what we want. Note that #12222 changed most geometric figures to be "1" but other things to be auto, and then #12581 switched contour and region plots to "1". Now we are suggesting changing density and matrix plots to be auto, not 1. And what about slope/vector fields? Historically we have kept such things at "1" because we figured the typical user would have them be squares. But maybe that was wrong. The canonical example is

        sage: x,y = var('x,y')
        sage: contour_plot(x^2+y^2-2,(x,-1,1), (y,-1,1))

and we went with that being circles, not ellipses :)

Jason, do you have any comments on this? I would hate to change our defaults again and again; the most important thing here is allowing user control of the aspect ratio, so we could put in a decorator to keep these at aspect ratio 1 (though then ppurka would have to change the text of his test).

comment:6 in reply to: ↑ 5 Changed 4 years ago by ppurka

Replying to kcrisman:

This looks great in general, except it does change density and matrix plots to having the default aspect ratio be "automatic".

This is the same as it was earlier! The aspect ratio is set to self.aspect_ratio() if it is None (c.f. the patch). And the latter returns "automatic" by default. So, if matrix and density plots have not been setting the aspect ratio to one explicitly, then they are running with "automatic" aspect ratio.

So, IMHO, for the plots where the aspect ratio is explicitly set to 1 by @options, it remains 1. And for the plots where it is not automatically set, "automatic" was already the default, so we are not changing anything here.

As a very gentle review request, could you remove the extra back tick from here? Same file. You didn't introduce it but it would be nice not to open a separate ticket for that.

Done.

comment:7 follow-up: Changed 4 years ago by kcrisman

Try a weird density plot without the patch and then with it. I at least think they look different - which is why the original poster had trouble in the first place.

comment:8 in reply to: ↑ 7 Changed 4 years ago by ppurka

Replying to kcrisman:

Try a weird density plot without the patch and then with it. I at least think they look different - which is why the original poster had trouble in the first place.

I realize that the problem showed up in the weird plots. My interpretation of it is this - if we set the aspect ratio to automatic before the plot, then matplotlib has no idea of the kind of data it is about to get. I don't know what it does internally. But if we set it after the plot then matplotlib has the data and now it can set the plot parameters appropriately.

Ideally, matplotlib should "keep in mind" that the aspect ratio is automatic and needs to be readjusted after the plot.

comment:9 Changed 4 years ago by ppurka

  • Report Upstream changed from N/A to Reported upstream. No feedback yet.

I have opened a bug upstream.

comment:10 follow-up: Changed 4 years ago by kcrisman

Wow, great tracking down!

That doesn't change that we should have the discussion about whether density plots should have automatic or 1 aspect ratio. I don't exactly have a horse in the race, but I think that needless API changes are also annoying.

comment:11 Changed 4 years ago by ppurka

  • Report Upstream changed from Reported upstream. No feedback yet. to Reported upstream. Developers deny it's a bug.

comment:12 in reply to: ↑ 10 Changed 4 years ago by ppurka

Replying to kcrisman:

That doesn't change that we should have the discussion about whether density plots should have automatic or 1 aspect ratio. I don't exactly have a horse in the race, but I think that needless API changes are also annoying.

About density plots: I think mpl is doing "the right thing" when aspect is set to auto. This is not a geometrical object, so I think there we should not have aspect 1 in all cases.

About matrix plots: In this case, I think we should interpret an unset aspect ratio as "equal" aspect ratio. This is already set explicitly in the code currently but this should be handled better. The current patch affects the matrix plots and the images are no longer square.

Other functions where aspect ratio is not set:

  • arrow - FancyArrowPatch from mpl has no aspect option
  • bezier_path - Path from mpl has no aspect option
  • complex_plot - the aspect ratio is not set in our code, and I think it is best left as automatic
  • plot_field - I don't see an aspect option in quiver of mpl
  • step - IMHO, there is no need for aspect ratio here
  • text.py - I think there is no need for aspect ratio here, and text in mpl has no such option either.

So, the only thing this patch affects are the matrix plots, and I think there the code should explicitly set the aspect ratio, i.e., we should introduce either @options(aspect_ratio=1) or @options(aspect_ratio="equal") in the definition of matrix_plot.

comment:13 Changed 4 years ago by ppurka

  • Branch set to u/ppurka/ticket/15315
  • Created changed from 10/22/13 01:35:45 to 10/22/13 01:35:45
  • Modified changed from 10/27/13 12:31:21 to 10/27/13 12:31:21

comment:14 Changed 4 years ago by ppurka

I added the option aspect_ratio=1 to matrix_plot since that is the desired default anyway.

comment:15 Changed 4 years ago by kcrisman

  • Commit set to bc5e5126ee9047f0ece1b321f13e735a2e06b52f

Note also that the mpl ticket has been closed as a wontfix.


New commits:

9980c6efix aspect_ratio for density_plot
bc5e512make sure that matrix plots have aspect ratio 1

comment:16 follow-up: Changed 4 years ago by kcrisman

Okay, why did my comment all of a sudden put "new commits" below it? I am confused.

comment:17 in reply to: ↑ 16 Changed 4 years ago by ppurka

Replying to kcrisman:

Okay, why did my comment all of a sudden put "new commits" below it? I am confused.

Those were the commits I added in comment:13. But due to some bug in trac it didn't appear immediately. It then appears in the next comment. For some reason it didn't appear in my comment. :)

I am not surprised that the mpl ticket has been closed. It requires a massive overhaul to fix it. And they are in the planning stages of that.

comment:18 Changed 4 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:19 Changed 3 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:20 Changed 3 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:21 Changed 3 years ago by kcrisman

Sorry for taking so very long to get to this... Just one dumb thing that is easy to fix:

+    Test that matrix plots have aspect ratio one (see :trac:`15315`)

needs extra double colon.

comment:22 Changed 3 years ago by git

  • Commit changed from bc5e5126ee9047f0ece1b321f13e735a2e06b52f to 2cc1beba873ee5898fadaa206cee23cd9be5455f

Branch pushed to git repo; I updated commit sha1. New commits:

a0316b8Merge branch 'u/ppurka/ticket/15315' of trac.sagemath.org:sage into ticket/15315
2cc1bebfix documentation string

comment:23 Changed 3 years ago by ppurka

Thanks. I had forgotten about these tickets.

comment:24 Changed 3 years ago by kcrisman

  • Status changed from needs_review to positive_review

comment:25 Changed 3 years ago by vbraun

  • Branch changed from u/ppurka/ticket/15315 to 2cc1beba873ee5898fadaa206cee23cd9be5455f
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.