Ticket #2364 (closed defect: fixed)

Opened 9 months ago

Last modified 1 month ago

[with patch, positive review] animate .show() method is poorly documented

Reported by: cwitty Assigned to: tba
Priority: major Milestone: sage-3.2
Component: documentation Keywords: animate, documentation, doctest
Cc:

Description

It should be better documented in animate.py how to specify the interframe delay and the number of iterations. At the very least, this should be described in the .show() docstring; better yet if it was also documented in the class docstring for Animation, which is what you see when you type:

sage: animate?

Attachments

2364.patch (16.7 kB) - added by jhpalmieri on 10/01/2008 01:57:12 PM.
2364-delta.patch (4.5 kB) - added by jhpalmieri on 10/22/2008 01:59:44 PM.
do not apply: this is only here to help the referee
2364-doctest-delta.patch (2.8 kB) - added by jhpalmieri on 10/28/2008 12:58:27 PM.
fix optional doctests
2364-new.patch (16.9 kB) - added by jhpalmieri on 10/28/2008 12:58:56 PM.
only apply this patch!

Change History

03/15/2008 07:23:52 PM changed by AlexGhitza

  • owner changed from was to tba.
  • component changed from algebraic geometry to documentation.

10/01/2008 01:57:12 PM changed by jhpalmieri

  • attachment 2364.patch added.

10/01/2008 02:07:15 PM changed by jhpalmieri

  • keywords set to animate, documentation, doctest.
  • summary changed from animate .show() method is poorly documented to [with patch, needs review] animate .show() method is poorly documented.

Here's a patch, based on 3.1.3.alpha2. I started working on animate.py before I knew about this ticket, so the patch does more than is required:

1. It improves the documentation for show and animate, as requested.

2. It adds docstrings and doctests to several functions for which they were missing; the file now has over 90% coverage. (Only __init__ is undocumented now.)

3. Many doctests used to be optional, things like

sage: a = animate([sin(x + float(k)) for k in srange(0,2*pi,0.3)], 
...                xmin=0, xmax=2*pi, figsize=[2,1])

These don't need to be optional -- the optional part comes in calls to a.show(), which calls the convert program -- so I've removed lots of optional tags. This way more of the code is actually doctested.

4. I also deleted one method: _set_axes. This method was undocumented. It was short and pretty simple. It was also called every time an animation was created; indeed, that was its only appearance in the code. So I just copied its contents (only 5 lines) to where it was called in the __init__ method.

10/01/2008 02:11:37 PM changed by jhpalmieri

5. Oh, one other thing: in the gif method (and hence in save and show which call it), I added a message saying where the file was being saved. Before, you would type

a.save('bozo.gif')

and, if you were using the notebook interface, the file would be saved something like 5 subdirectories below .sage. This is still true, but at least now you're told where the file is.

10/22/2008 12:40:52 PM changed by mhampton

  • summary changed from [with patch, needs review] animate .show() method is poorly documented to [with patch, mostly positive review] animate .show() method is poorly documented.

Although I too have been frustrated sometimes by the depth of the saves of things in sage, I don't think I like the solution here of always printing the path. I think this should be an option - perhaps a keyword/default like show_path = False, and if show_path were True then the path is displayed. That would also be useful for other saved graphics as well.

Otherwise this patch gets a very positive review; in general the show() documentation needs a lot of work and this is a great step in that direction.

10/22/2008 01:59:44 PM changed by jhpalmieri

  • attachment 2364-delta.patch added.

do not apply: this is only here to help the referee

10/22/2008 02:04:46 PM changed by jhpalmieri

Here are two new patches to deal with mhampton's comments. The one which should be applied is 2364-new.patch. The other patch, 2364-delta.patch, shows the difference between the old patch and the new patch. This way mhampton (for example) can referee the new patch more easily, I hope.

(That is, if you apply the original patch and then 2364-delta.patch, it should give the same result as just applying the new patch. I'm trying to achieve ease of refereeing as well as ease of incorporating the patch...)

(follow-up: ↓ 7 ) 10/28/2008 05:49:43 AM changed by mabshoff

  • summary changed from [with patch, mostly positive review] animate .show() method is poorly documented to [with patch, needs work] animate .show() method is poorly documented.

I don't particularly like the delta patch, i.e. the test file generated should be saved in SAGE_TMP for example since the $SAGE_ROOT tree or cwd might not be writable.

Cheers,

Michael

(in reply to: ↑ 6 ; follow-up: ↓ 8 ) 10/28/2008 07:46:51 AM changed by jhpalmieri

Replying to mabshoff:

I don't particularly like the delta patch, i.e. the test file generated should be saved in SAGE_TMP for example since the $SAGE_ROOT tree or cwd might not be writable.

I didn't change the location of the file -- as far as I know it's always been this way. Given this, it seems like the patch does not make anything worse, and for the most part improves the original file. I think the issue about the location of the save file should be a new ticket.

(in reply to: ↑ 7 ) 10/28/2008 07:49:58 AM changed by mabshoff

Replying to jhpalmieri:

I didn't change the location of the file -- as far as I know it's always been this way. Given this, it seems like the patch does not make anything worse, and for the most part improves the original file. I think the issue about the location of the save file should be a new ticket.

Sure. The issue will pop up once somebody runs doctests as non-owner. I don't particularly care if that issue gets fixed now or not, so a new ticket would get this patch a positive review.

Cheers,

Michael

(follow-up: ↓ 10 ) 10/28/2008 08:01:53 AM changed by jhpalmieri

Unfortunately (?), doctests won't catch it, since these commands are all optional -- they all rely on the convert command being present.

John

(in reply to: ↑ 9 ) 10/28/2008 08:06:04 AM changed by mabshoff

Replying to jhpalmieri:

Unfortunately (?), doctests won't catch it, since these commands are all optional -- they all rely on the convert command being present. John

Yes, but someone [can you guess? :)] has started running Sage with "-t -long -optional", so in the future we will catch this. I am all for merging this patch after someone verifies that the animate command still works as expected.

Cheers,

Michael

(follow-up: ↓ 12 ) 10/28/2008 11:43:31 AM changed by jhpalmieri

Oh dear. I don't know how to write the doctests, then. For example, I have a doctest which shows what happens when convert is missing:

        If ImageMagick is not installed, you will get an error message:
            sage: a.gif()       # optional
            /usr/local/share/sage/local/bin/sage-native-execute: 8: convert: not
            found

            Error: ImageMagick does not appear to be installed. Saving an
            animation to a GIF file or displaying an animation requires
            ImageMagick, so please install it and try again.

            See www.imagemagick.org, for example.

This will fail if you run the optional doctests with convert installed. Should I delete the doctest and just display the error message?

Also, I have doctests like this

            sage: a.save(show_path=True)  # optional
            Animation saved to file /home/isaac/.sage/sage0.gif.

in which I have inserted an invented pathname. What should I do about these?

(in reply to: ↑ 11 ) 10/28/2008 11:56:14 AM changed by mabshoff

Replying to jhpalmieri:

Oh dear. I don't know how to write the doctests, then. For example, I have a doctest which shows what happens when convert is missing: {{{ If ImageMagick? is not installed, you will get an error message: sage: a.gif() # optional /usr/local/share/sage/local/bin/sage-native-execute: 8: convert: not found Error: ImageMagick? does not appear to be installed. Saving an animation to a GIF file or displaying an animation requires ImageMagick?, so please install it and try again. See www.imagemagick.org, for example. }}} This will fail if you run the optional doctests with convert installed. Should I delete the doctest and just display the error message?

Nope, if someone runs optional doctests and the binary required is not there it will blow up. Nothing can change that until we have a more clever "#optinal" doctest treatment.

Also, I have doctests like this {{{ sage: a.save(show_path=True) # optional Animation saved to file /home/isaac/.sage/sage0.gif. }}} in which I have inserted an invented pathname. What should I do about these?

This would need to be changed to

"Animation saved to file .../sage0.gif."

Cheers,

Michael

10/28/2008 12:58:27 PM changed by jhpalmieri

  • attachment 2364-doctest-delta.patch added.

fix optional doctests

10/28/2008 12:58:56 PM changed by jhpalmieri

  • attachment 2364-new.patch added.

only apply this patch!

10/28/2008 01:04:12 PM changed by jhpalmieri

  • summary changed from [with patch, needs work] animate .show() method is poorly documented to [with patch, needs re-review] animate .show() method is poorly documented.

Here are two patches, fixing the optional doctests. Now sage -t -optional animate.py works on my machine.

The patch 2364-doctest-delta.patch shows the differences between the previous patch and this one: only a few doctests were changed.

The patch 2364-new.patch incorporates all of the patches into one file (so if you apply 2364.patch, 2364-delta.patch, and 2364-doctest-delta.patch, you will get the same as if you just applied this one).

So: either the two delta patches need to be refereed (since 2364.patch has received an essentially positive review), or 2364-new.patch needs to be refereed.

10/30/2008 01:39:29 AM changed by mabshoff

  • milestone changed from sage-3.2.1 to sage-3.2.

Bug Day 15 review material. This patch could bit rot easily, so let's get it in.

Cheers,

Michael

10/30/2008 05:21:12 AM changed by mhampton

  • summary changed from [with patch, needs re-review] animate .show() method is poorly documented to [with patch, positive review] animate .show() method is poorly documented.

Thanks for incorporating all those changes. I think this looks very good now. Optional tests pass on my machine.

10/30/2008 09:20:10 AM changed by mabshoff

  • status changed from new to closed.
  • resolution set to fixed.

Merged 2364-new.patch in Sage 3.2.alpha2