Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix contour and other plot default aspect ratio #12581

Closed
kcrisman opened this issue Feb 24, 2012 · 11 comments
Closed

Fix contour and other plot default aspect ratio #12581

kcrisman opened this issue Feb 24, 2012 · 11 comments

Comments

@kcrisman
Copy link
Member

On Feb 24, 3:04 pm, kcrisman <kcris...@gmail.com> wrote:
> Jason, what's going on with
> 
> x,y = var('x,y')
> contour_plot(x^2+y^2-2,(x,-1,1), (y,-1,1))

This was due to https://github.com/sagemath/sage-prod/issues/12213, and not fixed by #12222, because that only focused on shape primitives.  

The fix is most likely to add an option in the decorators of all the contour_plot.py commands to put aspect ratio back to 1 for those things.

Critical because we have in the past long discussed this; I would like to make it blocker since

This should plot concentric circles centered at the origin::

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

doesn't look like circles, but I suppose that they are circles...

Apply attachment: trac_12581-aspect_ratio.patch.

Depends on #9744

CC: @jasongrout @williamstein @benjaminfjones @mboratko

Component: graphics

Author: Karl-Dieter Crisman

Reviewer: Benjamin Jones, David Loeffler

Merged: sage-5.0.beta8

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

@kcrisman

This comment has been minimized.

@kcrisman
Copy link
Member Author

comment:1

This might depend on #9744. I don't think it touches the same parts of contour_plot.py, but if you get a problem applying, then it does. Note that we don't need the option for implicit_plot because it always gives a region plot or a contour plot.

For reviewing; please test whether this breaks anything from #11963. I don't quite understand what the 'correct' summation behavior is yet. I assume that this works okay, though.

And a question; should it be 1.0 instead of 1?

@benjaminfjones
Copy link
Contributor

comment:3

It looks like your patch applies over #9744 with a bit of fuzz, but it doesn't apply to a clean 5.0.beta5, so I changed the dependencies. I looked through the live docs for contour_plot with the patch applied and things look much better now. I don't see any problems. I haven't looked at whether anything from #11963 breaks yet..

FWIW, there are 6 places in the Sage library .py files where aspect_ratio=1.0 is used, and 46 places where aspect_ratio=1 is used (thank you, ack).

@benjaminfjones
Copy link
Contributor

Dependencies: #9744

@benjaminfjones
Copy link
Contributor

Reviewer: Benjamin Jones

@kcrisman
Copy link
Member Author

Attachment: trac_12581-aspect_ratio.patch.gz

@kcrisman
Copy link
Member Author

Author: Karl-Dieter Crisman

@kcrisman
Copy link
Member Author

comment:4

Okay, I updated it to officially depend on #9744 without fuzz.

@loefflerd
Copy link
Mannequin

loefflerd mannequin commented Mar 10, 2012

comment:5

Looks good.

@loefflerd
Copy link
Mannequin

loefflerd mannequin commented Mar 10, 2012

Changed reviewer from Benjamin Jones to Benjamin Jones, David Loeffler

@jdemeyer
Copy link

Merged: sage-5.0.beta8

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants