aspect ratio is not handled correctly in combined plots

[novoselt]

Using 4.7.2.rc0, I get the following behaviour:

p = circle((1,4), 3)
p.show() # Shows "round" circle
p += line([(3,4), (5,6)])
print p.aspect_ratio() # Outputs 1
p.set_aspect_ratio(1) # This should definitely make it 1
p.show() # Shows a distorted circle
p.show(aspect_ratio=1) # Shows "round" circle

In 4.7.1 everything behaves as expected, i.e. the first plot is not round, but two others are.

It may be related to #2100. I tried to look into it, but I don't quite understand how and where __aspect_ratio is taken into account internally. Do you guys have any clue?

CC: @kcrisman @jasongrout @sagetrac-ryan

Component: graphics

Author: Jason Grout

Reviewer: Dan Drake

Merged: sage-4.8.alpha3

Issue created by migration from https://trac.sagemath.org/ticket/11963

[jhpalmieri]

comment:1

Notice:

sage: p = circle((1,4), 3)
sage: p._extra_kwds
{}
sage: q = line([(3,4), (5,6)])
sage: q._extra_kwds
{'aspect_ratio': 'automatic'}

So when you add p and q, the _extra_kwds dicts get combined, so the automatic aspect ratio for q overrides the setting for p.

There are lots of different settings for plots, and they don't seem to be stored in consistent ways: some are in this _extra_kwds dict, some (like __aspect_ratio) are stored as attributes, and some (like "aspect ratio") are stored in more than one way, not necessarily consistently.

[novoselt]

comment:2

I am fine with overwriting settings - whatever is the aspect ratio for the combination is fine (or is a matter of taste). But when I explicitly set the aspect ratio to 1 after recombination and it does not work - it is a bug!

[jhpalmieri]

comment:3

Oh, I agree that it's a bug. It just might be hard to fix, or at least to fix properly.

[jasongrout]

comment:4

Thanks for diagnosing this, jhpalmieri. I think the attached patch takes care of the code changes. Some docs might need to be revised somewhere, though (like the aspect_ratio or set_aspect_ratio functions?), so I'm setting this as needs work. Anyone is welcome to help finish it!

[jasongrout]

comment:7

How come I can't go from 'new defect' directly to 'needs work'? I thought I could before.

[jdemeyer]

Milestone sage-4.7.3 deleted

[jasongrout]

comment:9

Attachment: trac-11963-fix-aspect-ratio.patch.gz

I think I covered the bases, so setting this as 'needs review'. Karl-Dieter or someone else, do you want to review this?

[jasongrout]

comment:10

Just FYI, I was testing this on a (slightly modified) 4.7.2.alpha3.

[dandrake]

comment:11

This fixes the problems I recently posted about on sage-devel, and works as advertised. Doctests and documentation are good. Positive review.

[dandrake]

Reviewer: Dan Drake

[dandrake]

Author: Jason Grout

[jdemeyer]

Merged: sage-4.8.alpha3

[williamstein]

comment:13

I'm very, very unhappy with the impact of this patch. See #12213.

[kcrisman]

comment:14

I'm very, very unhappy with the impact of this patch. See #12213.

Just a thought - maybe this could be reverted and put to 'needs work'? I guess that's up to the release manager, but seems easier than tracking down how to fix the example in #12213.

[jasongrout]

comment:15

Please don't revert this; at least, test the solution up on #12213 first. This patch fixes a rather serious problem with graphics.