Opened 9 years ago

Closed 9 years ago

#9746 closed enhancement (fixed)

documentation for plotting

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

Description

I went through the "live" documentation for plots and found a number of places that computations should be split into separate cells, plots weren't displayed, etc. This patch fixes these areas.

Attachments (2)

trac-9746-plot-doc-improvements.patch (11.7 KB) - added by jason 9 years ago.
9746-review.patch (1.9 KB) - added by jason 9 years ago.
apply on top of previous patches

Download all attachments as: .zip

Change History (21)

comment:1 Changed 9 years ago by jason

Depends on #9740, and possibly on #9221

comment:2 Changed 9 years ago by jason

  • Status changed from new to needs_review

comment:3 Changed 9 years ago by jason

  • Type changed from defect to enhancement

comment:4 follow-up: Changed 9 years ago by kcrisman

There is a typo of 'primitve' in plot/plot.py. Does this still apply to 4.6.alpha1?

Changed 9 years ago by jason

comment:5 in reply to: ↑ 4 ; follow-up: Changed 9 years ago by jason

Replying to kcrisman:

There is a typo of 'primitve' in plot/plot.py. Does this still apply to 4.6.alpha1?

Yes. I also refreshed the patch to correct the typo. This depends on #9740. Do you have time to review that and this patch?

comment:6 in reply to: ↑ 5 Changed 9 years ago by kcrisman

  • Status changed from needs_review to needs_work

Replying to jason:

Replying to kcrisman:

There is a typo of 'primitve' in plot/plot.py. Does this still apply to 4.6.alpha1?

Yes. I also refreshed the patch to correct the typo. This depends on #9740. Do you have time to review that and this patch?

Not totally at this time, though I see some of the things it is trying to fix. I did take a look at some things while checking out the new matplotlib, and noticed that the example with plot(1.5/(1+e^(-x)) is not correctly formatted in the documentation, and doesn't become a live cell. I'm sorry to make this a long process.

comment:7 follow-up: Changed 9 years ago by kcrisman

(Also, there are lots of places where there are many specific examples all together, such as in point?

point((1,2))
point((1,2,3))
point([(0,0), (1,1)])
point([(0,0,1), (1,1,1)]

but that should be a separate ticket. I may be responsible for some of those, not realizing the live documentation needed these separate - or does it?)

comment:8 in reply to: ↑ 7 Changed 9 years ago by jason

Replying to kcrisman:

(Also, there are lots of places where there are many specific examples all together, such as in point?

point((1,2))
point((1,2,3))
point([(0,0), (1,1)])
point([(0,0,1), (1,1,1)]

but that should be a separate ticket. I may be responsible for some of those, not realizing the live documentation needed these separate - or does it?)

Those are fixed in my version (after #9740, #9746, and #4342).

comment:9 Changed 9 years ago by kcrisman

Hilarious - I didn't make it far enough down to see that this exact one is on this ticket! Well, like I said, I don't have time to check it all the way... I feel like there were other similar places, though.

comment:10 Changed 9 years ago by jason

  • Status changed from needs_work to needs_review

I've attached a patch which takes care of the error pointed out above, and corrects two or three more things in the docs.

Changed 9 years ago by jason

apply on top of previous patches

comment:11 Changed 9 years ago by jason

ptestlong in 4.6.alpha1 (Ubuntu 10.04 64-bit) passes with the following tickets applied in order: #9221 (and new spkg), #9740, #9746, #4342.

comment:12 Changed 9 years ago by kcrisman

  • Status changed from needs_review to needs_work

Okay, here are comments. A lot of them are very similar in style, but I tried to be exhaustive within the files you modified. I am too tired to look for problems in the other files, though they are likely there and I likely introduced them :) But overall this is good, I just found more of the same for most (a few other errors, though). Here we go!

In scatterplot, there is another one of those 'these are equivalent' things, but they're not separated out. Also, I get code{scatter_plot.options} instead of the actual code. Should it be show() or show()?

In sage.plot.polygon.polygon we have something similar in the examples - somehow you only got some of them. Again with the extra options guy, too.

Same with the equivalent in point.py, in both point and points.

In the plot_field.py file, why did you once add the x,y variable declaration and once not? It doesn't really matter to me, but I wonder if there is something I'm missing. Again with the show() or show(), and the equivalent. It's not so important to me with the equivalent showing two things, but I feel like maybe you changed that one place - or maybe not.

In plot.py, ironically, just above the place where you fixed the 1.5/(1+e^(-x)) thing, there are a bunch of plots I didn't separate in my custom ticks patch. My apologies - but there they are! I also still get an error 'ellipsis object not callable' or something in 'add grid lines at specific positions (using lists/tuples)'. There's an ellipsis that got stuck in there still somehow - I think you got a different one of these.

In line.py, after the cool cat there are a couple things as in the previous files - one nonseparated, one equivalent issue/show() issue.

In disk.py, maybe the disk that is parallel to the xy-plane should be plotted, not just its type? Same equiv/show question.

I don't know what happened in density plot, but I think a tick is missing in the DensityPlot documentation - likely my fault? This is in 'Examples'.

In contour plot, the very last example under region_plot should have two plots, but has one. But they are different.

The circles also has the parallel to the xy-plane issue when it comes to giving the type, but not the plot.

comment:13 Changed 9 years ago by kcrisman

  • Reviewers set to Karl-Dieter Crisman

comment:14 Changed 9 years ago by jason

  • Status changed from needs_work to needs_review

So, since a lot of tickets depend on this one, why don't we make all of these a separate ticket? That's a great list for improving docs!

comment:15 follow-up: Changed 9 years ago by kcrisman

  • Status changed from needs_review to positive_review

Okay, I'll cave in. I don't know that 'a lot' of tickets depend on this (if #4342 counts as 'a lot') but a ticket with 'even more plot documentation improvements' sounds okay. This is now #10032.

Positive review to this patch, but noting that #10032 addresses more issues of a similar nature.

comment:16 in reply to: ↑ 15 ; follow-up: Changed 9 years ago by jason

Replying to kcrisman:

Okay, I'll cave in. I don't know that 'a lot' of tickets depend on this (if #4342 counts as 'a lot') but a ticket with 'even more plot documentation improvements' sounds okay. This is now #10032.

I knew it was in the stack of patches I had applied, and other patches depended on it. How about "an important patch that has been waiting a very long time depends on this"? :)

My purpose is to just narrow the scope of this ticket to exactly the improvements actually done in the patch. Thanks for agreeing and opening up another ticket for more improvements.

comment:17 in reply to: ↑ 16 Changed 9 years ago by kcrisman

Replying to jason:

Replying to kcrisman:

Okay, I'll cave in. I don't know that 'a lot' of tickets depend on this (if #4342 counts as 'a lot') but a ticket with 'even more plot documentation improvements' sounds okay. This is now #10032.

I knew it was in the stack of patches I had applied, and other patches depended on it. How about "an important patch that has been waiting a very long time depends on this"? :)

I agree with that!

My purpose is to just narrow the scope of this ticket to exactly the improvements actually done in the patch. Thanks for agreeing and opening up another ticket for more improvements.

No problem - although not using queues made this a little harder for me to test.

comment:18 Changed 9 years ago by kcrisman

This one

 I also still get an error 'ellipsis object not callable' or something in 'add grid lines at specific positions (using lists/tuples)'. There's an ellipsis that got stuck in there still somehow - I think you got a different one of these.

in particular is aggravating, and should be fixed quickly, if possible.

comment:19 Changed 9 years ago by mpatel

  • Merged in set to sage-4.6.alpha3
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.