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

make it so plot(...) passes extra options to show (maybe only those that makes sense) #5651

Closed
williamstein opened this issue Mar 31, 2009 · 19 comments

Comments

@williamstein
Copy link
Contributor

This works now:

plot(sin(x^2),(x,-3,3)).show(figsize=[8,2])

This would be nice:

plot(sin(x^2),(x,-3,3),figsize=[2,8])

The analogue of the above works systematically everywhere for 3d plotting.

CC: @sagetrac-wcauchois @williamstein @jasongrout

Component: graphics

Author: Bill Cauchois

Reviewer: William Stein, Jason Grout

Merged: sage-4.1.1.alpha0

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

@williamstein
Copy link
Contributor Author

comment:2

This is really awesome. However, things like this should work too:

sage: line([(0,1), (3,4)],figsize=[10,2])
Traceback (most recent call last):
...
RuntimeError: Error in line(): option 'figsize' not valid.

Also, this should have gridlines:

plot(sin(x^2),(x,-3,3),gridlines=True) + plot(cos(x^2),(x,-3,3))

Clarify the comment

# If you add parameters to show(), you should update the code below. 

}}}

@sagetrac-wcauchois
Copy link
Mannequin

sagetrac-wcauchois mannequin commented May 11, 2009

comment:3

Now it works for everything, ever!! With doctests too.

@williamstein
Copy link
Contributor Author

comment:4

Positive review modulo making so consistent with 3d plotting:

sphere((0,0,0),1, aspect_ratio=[1,4,7]) + sphere((1,1,1),1, aspect_ratio=[1,1,1])

Note that it is the rightmost thing that has precedence. Otherwise positive review.

@sagetrac-wcauchois
Copy link
Mannequin

sagetrac-wcauchois mannequin commented May 14, 2009

Attachment: trac5651.patch.gz

@sagetrac-wcauchois
Copy link
Mannequin

sagetrac-wcauchois mannequin commented May 14, 2009

comment:5

William,
I fixed the precedence issue.

@sagetrac-mabshoff
Copy link
Mannequin

sagetrac-mabshoff mannequin commented May 15, 2009

comment:7

This one needs to be rebased due to a doctest merge conflict in arrow.py. On top of that: this is a diff, so please make sure you post a proper patch this time.

mabshoff@sage:/scratch/mabshoff/sage-4.0.alpha0/devel/sage$ patch -p1 --dry-run < trac_5651.patch 
patching file sage/plot/arrow.py
Hunk #1 FAILED at 310.
1 out of 1 hunk FAILED -- saving rejects to file sage/plot/arrow.py.rej
patching file sage/plot/bar_chart.py
patching file sage/plot/bezier_path.py
<SNIP>

Note that various doctest patches in plot/* are going into 4.0.alpha0, so please rebase post 4.0.a0.

Cheers,

Michael

@sagetrac-mabshoff sagetrac-mabshoff mannequin changed the title make it so plot(...) passes extra options to show (maybe only those that makes sense) [needs rebase] make it so plot(...) passes extra options to show (maybe only those that makes sense) May 15, 2009
@sagetrac-mabshoff
Copy link
Mannequin

sagetrac-mabshoff mannequin commented May 15, 2009

comment:8

Note that with #6006, #6023 and #6030 applied there are two more small rejects.

Cheers,

Michael

@sagetrac-wcauchois
Copy link
Mannequin

sagetrac-wcauchois mannequin commented May 22, 2009

comment:9

I rebased the patch on Sage 4.0.rc0. As far as I could tell, the doctest failures I encountered were not related to this patch. My apologies for posting a diff beforehand, I'm still learning the idiosyncracies of Mercurial Queues :).

@sagetrac-wcauchois sagetrac-wcauchois mannequin changed the title [needs rebase] make it so plot(...) passes extra options to show (maybe only those that makes sense) make it so plot(...) passes extra options to show (maybe only those that makes sense) May 22, 2009
@mwhansen
Copy link
Contributor

mwhansen commented Jun 1, 2009

comment:12

Hi Bill,

I tried applying this to 4.0 and got a bunch of failures. I can probably rebase it later this evening.

@mwhansen mwhansen changed the title make it so plot(...) passes extra options to show (maybe only those that makes sense) [needs rebase] make it so plot(...) passes extra options to show (maybe only those that makes sense) Jun 1, 2009
@sagetrac-wcauchois
Copy link
Mannequin

sagetrac-wcauchois mannequin commented Jun 1, 2009

comment:13

I would appreciate that mhansen, thanks!

@sagetrac-wcauchois
Copy link
Mannequin

sagetrac-wcauchois mannequin commented Jul 1, 2009

comment:14

I added a new rebase, so if someone could review this then we can finally get this functionality into Sage.

I feel like I did a little bit of a dirty thing with the new linkmode parameter, which was added somewhere along the line. linkmode is intended to be consumed by show() before the keywords are passed onto save(), so I just popped it from the keyword dict at the beginning of the function.

@sagetrac-wcauchois sagetrac-wcauchois mannequin changed the title [needs rebase] make it so plot(...) passes extra options to show (maybe only those that makes sense) make it so plot(...) passes extra options to show (maybe only those that makes sense) Jul 1, 2009
@sagetrac-wcauchois
Copy link
Mannequin

sagetrac-wcauchois mannequin commented Jul 1, 2009

based on sage 4.1.alpha2 (fixed, whoops)

@jasongrout
Copy link
Member

comment:15

Attachment: trac5651-rebase.patch.gz

The rebased patch applies fine to my 4.1 tree. I tried a few examples and ran the doctests in plot/.py[x] and plot/plot3d/.py[x] and everything seems fine. I'm giving a positive review to the rebasing. That combined with William's almost positive review above adds up to a positive review.

I also looked at the code and it looked reasonable.

Thanks and good work!

@jasongrout
Copy link
Member

Reviewer: William Stein, Jason Grout,

@sagetrac-mvngu
Copy link
Mannequin

sagetrac-mvngu mannequin commented Jul 19, 2009

Author: Bill Cauchois

@sagetrac-mvngu
Copy link
Mannequin

sagetrac-mvngu mannequin commented Jul 19, 2009

Merged: sage-4.1.1.alpha0

@sagetrac-mvngu
Copy link
Mannequin

sagetrac-mvngu mannequin commented Jul 19, 2009

comment:16

Merged trac5651-rebase.patch. That rebased patch contains doctrings that doesn't conform with conventions for formatting docstrings. In particular, in sage/plot/bar_chart.py:

131	    Extra options will get passed on to show(), as long as they are valid:

In sage/plot/bezier_path.py:

171	    Extra options will get passed on to show(), as long as they are valid:

In sage/plot/matrix_plot.py:

58	    Extra options will get passed on to show(), as long as they are valid:
62	    Extra options will get passed on to show(), as long as they are valid:

In sage/plot/polygon.py:

255	    Extra options will get passed on to show(), as long as they are valid:

These issues should be addressed in another enhancement ticket.

@sagetrac-mvngu sagetrac-mvngu mannequin closed this as completed Jul 19, 2009
@sagetrac-mvngu
Copy link
Mannequin

sagetrac-mvngu mannequin commented Jul 20, 2009

comment:17

See #6573 for an enhancement ticket.

@sagetrac-mvngu
Copy link
Mannequin

sagetrac-mvngu mannequin commented Jul 21, 2009

Changed reviewer from William Stein, Jason Grout, to William Stein, Jason Grout

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

3 participants