Animate example looks broken

[gagern]

There is one example used in several places in the documentation of module animate:

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

However, at least on my system the individual frames are of slightly different sizes, ranging from 184×84 to 184×90. And the image looks pretty strange as well:

This seems to be mostly due to the fact that the axis range, described by xmin and xmax, doesn't automatically fix a range for x. Instead, it seems as if x will automatically range from -1 to 1, but clipped to approximately the axis range.

My suggested remedy is to actually provide a range for x, and I'll upload a branch for this. I'll also address some other documentation and testsuite issues in this. Some of the tests take longer than one second, but aren't marked as long. One figure looks so small you can't see anything. And the information a user receives in response to animate? is pretty sparse, since most examples are at the module level. So I introduced a cross reference to that.

This ticket is a spin-off from some work I did for #16533.

CC: @nilesjohnson

Component: graphics

Author: Martin von Gagern

Branch: 9d9e590

Reviewer: Jakob Kroeker, Karl-Dieter Crisman

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

[gagern]

Attachment: broken-example.gif

Current broken state of the example

[gagern]

comment:1

Including image in the ticket description.

[gagern]

Branch: u/gagern/ticket/16570

[gagern]

New commits:

04665b0	Fix frame size for standard example animation.
368dca2	Mark some animation tests as long time.
d515502	Minor improvements to aminate documentation.

[gagern]

Commit: d515502

[ppurka]

comment:5

Can you add a comment about frame size in the documentation itself to warn the user of bad examples? Maybe you can even refer to this ticket.

[gagern]

comment:6

Replying to @ppurka:

Can you add a comment about frame size in the documentation itself to warn the user of bad examples?

Reasonable suggestion, but I'm somewhat uncertain where to put this. I guess it would be best to put this into the docstring of the whole module. There it might go after the warning about the need for ImageMagick or FFmpeg, or as one of the examples, towards the end.

As a side note, my code from #16571 introduces a doc test which exhibits the APNG generator complaining about incorrect sizes. Mainly to help people understand the error message issued in this case.

Maybe you can even refer to this ticket.

I'd probably word this as follows:

“Note that it is your responsibility to provide frames with matching sizes. Otherwise the result is undefined: the animation might fail to save, fail to play back, or simply look strange. Most often you'll want to ensure common coordinate systems as well, so passing xmin, xmax, ymin, ymax as parameters is quite common. Prior to trac ticket !#16570, one of the running examples in the documentation violated this requirement:”

Then I'd paste the broken example, but only the construction, without the command to display it. That way there is not much work for the doctests, and we can add checks for common frame size to the implementation later on without breaking that doctest.

In the long run, a better alternative might be not to warn users but to avoid the unexpected behavior by choosing a common coordinate system automatically. But that will be more work. I just filed #16654 to keep track of that idea.

[nilesjohnson]

comment:7

This looks good to me -- I wouldn't worry about the additional warning. Unfortunately I won't have time to give it a proper review for some weeks.

[sagetrac-jakobkroeker]

comment:9

I'm about to review this one and have some questions:

• what should the line

 a.show() # optional -- ImageMagick

do? (I have ImageMagick installed), but nothing happens: nothing is displayed in the notebook and no error occurs.
However, I can save the gif and then look at it using a viewer, e.g. 'gthumb'

• how to show the animation in the notebook (if at all?)

There is also a (unchanged) line in the doctest
a.gif() # not tested

Q: what does that mean?

What should I look at in addition?

[jhpalmieri]

comment:10

The issue with a.show() producing no output in the notebook should have been fixed by #16608, merged in Sage 6.3.beta6. What version of Sage are you using?

[sagetrac-jakobkroeker]

comment:11

What version of Sage are you using?

If I didn't miss or screw op something, I just checked out the ticket.
If I remember correctly, 'sage' showed me that I'm on 6.3.beta4.

Probably I should rebase first, or someone else should rebase the ticket?

[jhpalmieri]

comment:12

Let's view testing it and rebasing it as two separate tasks. To test: if your develop branch is up-to-date, you should be able to check out the branch for this ticket, type git merge develop (then make), and then see if things here work the way they're supposed to. I think we can leave rebasing this ticket to the authors, and we can leave it until later, unless the merge doesn't work.

[gagern]

comment:13

Since #16571 and (by transitivity) #16650 depends on this, rebasing would mean changes to those dependent tickets. I'd prefer the merge instead. I'm building 6.3 just now, and once that's done, I'll try the merge and push if that works as expected.

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

050c3b8

Merge tag '6.3' into ticket/16570

[sagetrac-git]

Changed commit from d515502 to 050c3b8

[gagern]

comment:15

Replying to @sagetrac-jakobkroeker:

Thanks for reviewing this. As already mentioned, the a.show() line should work again now, displaying the image inside the notebook. The a.gif() line should show the animation as well, thanks to #16645. The latter is not run as part of the automated doctests, but I'd say please include this in your manual test, since it should work in the notebook. On the other hand, since I don't touch that line, if it doesn't work that's likely best dealt with in a separate ticket.

[sagetrac-jakobkroeker]

Reviewer: Jakob Kroeker

[sagetrac-jakobkroeker]

comment:17

good, after I updated the ffmpeg 0.6.5 manually on my machine (Scientific Linux 6.5), the doctests passed!

Otherwise ffmpeg 0.6.5. complains about the 'long' parameter argument. This might be a different new(?) issue, since several universities have SL installations. (SL packages are dated)

All fixed examples worked as expected. However, there are two other broken animate examples;
would you mind to fix them in this ticket? (see below)

What also should be changed (maybe also in this ticket, since it is a pretty small change),
is storing the name of the temporary directory in variable named 'dir'. 'dir' is a keyword/a method
and should not be overwritten

############# # problem with the varying axis range #############  
#################### line 417

a = animate([plot(x^2 + n) for n in range(4)])
a.show()

#################### line 449
E = EllipticCurve('37a')
v = [E.change_ring(GF(p)).plot(pointsize=30) for p in [97, 101, 103, 107]]
a = animate(v, xmin=0, ymin=0)
a
a.show() # optional -- ImageMagick

There is also one example with unreadable axis labels:

##############   problem with the axis labels - unreadable #############  
############# line 294
a = animate([circle((i,0),1) for i in srange(0,2,0.4)],
                xmin=0, ymin=-1, xmax=3, ymax=1, figsize=[2,1])
a.show()        # optional -- ImageMagick
b = animate([circle((0,i),1,hue=0) for i in srange(0,2,0.4)],
                xmin=0, ymin=-1, xmax=1, ymax=3, figsize=[1,2])
b.show()        # optional -- ImageMagick

and two examples with unfortunate axis range, but that is probably discussable:

#############   image section not optimal: #############  
#############  line 326
a = animate([circle((i,0),1,thickness=20*i) for i in srange(0,2,0.4)],
                xmin=0, ymin=-1, xmax=3, ymax=1, figsize=[2,1], axes=False)
a.show()             # optional -- ImageMagick
b = animate([circle((0,i),1,hue=0,thickness=20*i) for i in srange(0,2,0.4)],
                xmin=0, ymin=-1, xmax=1, ymax=3, figsize=[1,2], axes=False)
b.show()             # optional -- ImageMagick
p = a*b              # indirect doctest
len(a), len(b)
len(p)
(a*b).show()         # optional -- ImageMagick

#################### line 352
a = animate([circle((i,0),1,thickness=20*i) for i in srange(0,2,0.4)],
             xmin=0, ymin=-1, xmax=3, ymax=1, figsize=[2,1], axes=False)
a.show()

[nilesjohnson]

comment:26

Replying to @gagern:

Replying to @nbruin:

[…] this behaviour is a regression. I happened to try the example in 5.10 and there the animation appeared as one would expect. So it seems that xmin and xmax were used as bounds for x at some point before.

After almost two days of bisecting, I got the culprit: it's #12827 – again.

Bummer. Thanks for investigating; I will look into this too.

[gagern]

Changed branch from u/jakobkroeker/ticket/16570 to u/gagern/ticket/16570

[gagern]

comment:28

OK, things look good if I simply pass the keywords to the plot function.

---

New commits:

403a9e6	animate: Pass keyword arguments to plot constructor.
9e4d39c	Merge tag '6.4.beta4' into ticket/16570
e14ebdb	Mark some animation tests as long time.
17d4f90	Minor improvements to aminate documentation.
07eaa4c	Improve some more animation examples.

[gagern]

Changed commit from 9df42a4 to 07eaa4c

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

9d9e590

fixed max y-axis range to 4 for the x^2 example

[sagetrac-git]

Changed commit from 07eaa4c to 9d9e590

[kcrisman]

comment:30

Is there any further review needed here? It seems like you all have reviewed each other's work, except maybe the keyword bit, which is fine. Tests pass (I don't have ffmpeg but I don't think anything really changed there.)

[gagern]

comment:31

Replying to @kcrisman:

Is there any further review needed here? It seems like you all have reviewed each other's work, except maybe the keyword bit, which is fine.

As far as I see it, 403a9e6 with the keyword argument handling was the only bit without review. If you consider it fine, feel free to name yourself a reviewer as well, and give this thing a positive review. I'd be glad for it.

[sagetrac-jakobkroeker]

comment:32

OK, things look good if I simply pass the keywords to the plot function

I'm not too familiar with sage/python,
but I guess, in make_image() **kwds should also be passed to save_image()?

[gagern]

comment:33

Replying to @sagetrac-jakobkroeker:

but I guess, in make_image() **kwds should also be passed to save_image()?

graphics.save (which is what save_image delegates to) will combine arguments from three sources: self.SHOW_OPTIONS which contains default values, self._extra_kwds which obtains keywords from the plot function, and the kwds passed to save itself. Looking at plot we see that all show-relevant keywords (as determined by Graphics._extract_kwds_for_show) will get passed to _set_extra_kwds except xmin and xmax which are explicitely ignored.

So what's the conclusion here? As long as a keyword argument is deemed suitable for show, i.e. matches a key in Graphics.SHOW_OPTIONS, then passing it as an agument to plot is enough, and we gain nothing by repeating those arguments for save. Are there any arguments which make sense for save but do not fall in that category? Since show is little more than a call to save followed by launching the viewer, this should in my opinion not be the case. I'd say there is no need to repeat the arguments, and if any arguments would be needed for show, then that would be a bug in its own right.

[kcrisman]

comment:34

OK, things look good if I simply pass the keywords to the plot function

I'm not too familiar with sage/python,
but I guess, in make_image() **kwds should also be passed to save_image()?

In principle, the very first line in plot.plot being

G_kwds = Graphics._extract_kwds_for_show(kwds, ignore=['xmin', 'xmax'])

and near the end being

G._set_extra_kwds(G_kwds)

should suffice for that, but if you can think of a way this messes it up, that would be helpful.

Edit: looks like Martin and I agree on this.

[kcrisman]

Changed reviewer from Jakob Kroeker to Jakob Kroeker, Karl-Dieter Crisman

[sagetrac-jakobkroeker]

comment:36

G_kwds = Graphics._extract_kwds_for_show(kwds, ignore=['xmin', 'xmax'])

I have a question related to the xmin/xmax weirdness (my subjective impression), which has no influence on the positive review of this ticket:

• why is xmin and xmax ignored for G_kwds?
• there is xmin and xmax for starting and ending x value and same options names (for show() ) to limit the horizontal x-axis?
• is the xmin/xmax stuff (axis limit, start/end x value, ignored options) documentation for plot(), show() sufficient and understandable for users ?

[ppurka]

comment:37

See this #13368

[kcrisman]

comment:38

See this #13368

Yes, I agree, and now that I look at that again I agree with my former idea this was a bad cut-paste. Let's continue discussion there, if necessary.

[vbraun]

Changed branch from u/gagern/ticket/16570 to 9d9e590

[kcrisman]

Changed commit from 9d9e590 to none

[kcrisman]

comment:40

Possible bad outcome to this revealed in #18176.