Opened 9 years ago

Closed 9 years ago

Last modified 8 years ago

#11963 closed defect (fixed)

aspect ratio is not handled correctly in combined plots

Reported by: novoselt Owned by: jason, was
Priority: major Milestone: sage-4.8
Component: graphics Keywords:
Cc: kcrisman, jason, ryan Merged in: sage-4.8.alpha3
Authors: Jason Grout Reviewers: Dan Drake
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description

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?

Attachments (1)

trac-11963-fix-aspect-ratio.patch (5.8 KB) - added by jason 9 years ago.

Download all attachments as: .zip

Change History (16)

comment:1 Changed 9 years ago by jhpalmieri

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.

comment:2 Changed 9 years ago by novoselt

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!

comment:3 Changed 9 years ago by jhpalmieri

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

comment:4 Changed 9 years ago by jason

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!

comment:5 Changed 9 years ago by jason

  • Status changed from new to needs_review

comment:6 Changed 9 years ago by jason

  • Status changed from needs_review to needs_work

comment:7 Changed 9 years ago by jason

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

comment:8 Changed 9 years ago by jdemeyer

  • Milestone sage-4.7.3 deleted

Milestone sage-4.7.3 deleted

Changed 9 years ago by jason

comment:9 Changed 9 years ago by jason

  • Milestone set to sage-4.8
  • Status changed from needs_work to needs_review

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

comment:10 Changed 9 years ago by jason

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

comment:11 Changed 9 years ago by ddrake

  • Authors set to Jason Grout
  • Reviewers set to Dan Drake
  • Status changed from needs_review to positive_review

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

comment:12 Changed 9 years ago by jdemeyer

  • Merged in set to sage-4.8.alpha3
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:13 follow-up: Changed 8 years ago by was

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

comment:14 in reply to: ↑ 13 Changed 8 years ago by kcrisman

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.

comment:15 Changed 8 years ago by jason

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

Note: See TracTickets for help on using tickets.