#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)
Change History (16)
comment:1 Changed 9 years ago by
comment:2 Changed 9 years ago by
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
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
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
- Status changed from new to needs_review
comment:6 Changed 9 years ago by
- Status changed from needs_review to needs_work
comment:7 Changed 9 years ago by
How come I can't go from 'new defect' directly to 'needs work'? I thought I could before.
Changed 9 years ago by
comment:9 Changed 9 years ago by
- 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
Just FYI, I was testing this on a (slightly modified) 4.7.2.alpha3.
comment:11 Changed 9 years ago by
- 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
- Merged in set to sage-4.8.alpha3
- Resolution set to fixed
- Status changed from positive_review to closed
comment:13 follow-up: ↓ 14 Changed 9 years ago by
I'm very, very unhappy with the impact of this patch. See #12213.
comment:14 in reply to: ↑ 13 Changed 9 years ago by
comment:15 Changed 9 years ago by
Please don't revert this; at least, test the solution up on #12213 first. This patch fixes a rather serious problem with graphics.
Notice:
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.