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.

[gagern]

Current broken state of the example

[gagern]

Including image in the ticket description.

[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.

[ppurka]

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]

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.

[niles]

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.

[jakobkroeker]

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]

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?

[jakobkroeker]

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]

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]

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.

[git]

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

​050c3b8

Merge tag '6.3' into ticket/16570

[gagern]

Replying to 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.

[jakobkroeker]

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()

[git]

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

​a5e766d

Improve some more animation examples.

[gagern]

Replying to jakobkroeker:

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)

Sounds like a different issue, yes. We might have to implement some version detection there. Will you file a ticket for this? According to the ​FFmpeg News archive that version dates from January 2012.

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)

I did so in the commit I just pushed.

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

Overwriting ​the dir builtin within the tests causes no immediate harm. Only when users adapt these tests and therefore loose access to the builtin, then we'd have some impact. Nevertheless, I'll change the name to td.

I fixed the other plots you mentioned. In the case of a*b, the "unfortunate" range might in fact help demonstrate how this operation behaves in the presence of non-matching ranges, so I'd leave that as it is. And the last unfortunate range has no impact on the issue being demonstrated, so I'll leave that as well.

I hope you'll have time to give this a positive review, even though I took quite long to react to your input.

[jakobkroeker]

Except the x^2 plot (line 417, ff) everything is ok now. For that example ymax changes during the animation for me. If that is not by intention, I would set ymax=4 or similar.

Overwriting ​the dir builtin within the tests causes no immediate harm. Only when users adapt these tests and therefore loose access to the builtin, then we'd have some impact.

That is exactly why I issued the usage of dir !

Nevertheless, I'll change the name to td

in general I find self-descriptive naming like 'tempDir' better, but that is up to you if you want to keep 'td' or change it to something more descriptive.

[jakobkroeker]

Hello,

I set the ymax limit to 4 for the x^2 example:

​http://git.sagemath.org/sage.git/commit/?h=9df42a472cd18d9f7b0eaa3cf820c515f74e54a6&id=811ab08ce3e55c8314ddc81b73671b2cbf611786

---

New commits:

​ba4cfed	Merge branch 'develop' into ticket/16570
​3001502	Merge branch 'u/gagern/ticket/16570' of git://trac.sagemath.org/sage into ticket/16570
​811ab08	fixed max y-axis range to 4 for the x^2 example
​9df42a4	Merge branch 'u/gagern/ticket/16570' into ticket/16570

[nbruin]

I didn't see it remarked here, but 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.

It might be worth tracking down what caused the regression (perhaps check the history of the files where changes on this ticket were made) so that we make sure we're fixing the regression properly, not just partially patching up the issue.

[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.

​9eddf99 broke the example, causing error messages instead. By ​5a10336 the example could be executed again, but looked broken as described. Looking at ​the changes in between the issue becomes apparent. The __init__ method is changed like this:

-        w = []
-        for x in v:
-            if not isinstance(x, sage.plot.graphics.Graphics):
-                x = plot.plot(x, (kwds.get('xmin',0), kwds.get('xmax', 1)))
-            w.append(x)
-        if len(w) == 0:
-            w = [sage.plot.graphics.Graphics()]
-        self._frames = w
+        ## 
+        ## code for type-checking input -- seems like a bad idea
+        ##
+        # if typecheck_input:
+        #     w = []
+        #     for x in v:
+        #         if not is_graphic(x):
+        #             x = plot.plot(x, (kwds.get('xmin',0), kwds.get('xmax', 1)))
+        #         w.append(x)
+        #     if len(w) == 0:
+        #         w = [plot.Graphics()]
+        # else:
+        #     w = v
+        self._frames = v
         self._kwds = kwds

and as a replacement we get the new make_image method:

+        p = plot.plot(frame)
+        p.save_image(filename, **kwds)

If I move the **kwds from the sage_image call to the plot call, the example looks nice again. I guess I'll try this change and see how the rest of the animate test suite behaves for this.

[niles]

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]

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.

[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

[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. Tests pass (I don't have ffmpeg but I don't think anything really changed there.)

[gagern]

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.

[jakobkroeker]

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]

Replying to 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]

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.

[jakobkroeker]

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]

See this #13368

[kcrisman]

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.

[kcrisman]

Possible bad outcome to this revealed in #18176.