Opened 7 years ago

Closed 6 years ago

Last modified 6 years ago

#16570 closed defect (fixed)

Animate example looks broken

Reported by: gagern Owned by:
Priority: minor Milestone: sage-6.4
Component: graphics Keywords:
Cc: niles Merged in:
Authors: Martin von Gagern Reviewers: Jakob Kroeker, Karl-Dieter Crisman
Report Upstream: N/A Work issues:
Branch: 9d9e590 (Commits, GitHub, GitLab) Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by 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:

Current broken state of the example

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.

Attachments (1)

broken-example.gif (40.4 KB) - added by gagern 7 years ago.
Current broken state of the example

Download all attachments as: .zip

Change History (41)

Changed 7 years ago by gagern

Current broken state of the example

comment:1 Changed 7 years ago by gagern

  • Description modified (diff)

Including image in the ticket description.

comment:2 Changed 7 years ago by gagern

  • Branch set to u/gagern/ticket/16570
  • Created changed from 06/27/14 18:11:34 to 06/27/14 18:11:34
  • Modified changed from 06/27/14 18:14:15 to 06/27/14 18:14:15

comment:3 Changed 7 years ago by gagern

  • Commit set to d5155020dc2931d166f360f044a08b7b7bb78b24

New commits:

04665b0Fix frame size for standard example animation.
368dca2Mark some animation tests as long time.
d515502Minor improvements to aminate documentation.

comment:4 Changed 7 years ago by gagern

  • Status changed from new to needs_review

comment:5 follow-up: Changed 7 years ago by 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.

comment:6 in reply to: ↑ 5 Changed 7 years ago by 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.

comment:7 Changed 7 years ago by niles

  • Cc niles added

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.

comment:8 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

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

  • Status changed from needs_review to needs_info

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?

comment:10 Changed 7 years ago by 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?

comment:11 Changed 7 years ago by 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?

comment:12 Changed 7 years ago by 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.

Last edited 7 years ago by jhpalmieri (previous) (diff)

comment:13 Changed 7 years ago by 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.

comment:14 Changed 7 years ago by git

  • Commit changed from d5155020dc2931d166f360f044a08b7b7bb78b24 to 050c3b8afb736b20e46feb85f9273fba35336688

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

050c3b8Merge tag '6.3' into ticket/16570

comment:15 in reply to: ↑ 9 Changed 7 years ago by 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.

comment:16 Changed 7 years ago by gagern

  • Status changed from needs_info to needs_review

comment:17 follow-up: Changed 7 years ago by jakobkroeker

  • Reviewers set to Jakob Kroeker
  • Status changed from needs_review to needs_work

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

comment:18 Changed 7 years ago by git

  • Commit changed from 050c3b8afb736b20e46feb85f9273fba35336688 to a5e766dcccea56a5af130dc4f7cde649997dc646

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

a5e766dImprove some more animation examples.

comment:19 in reply to: ↑ 17 Changed 7 years ago by gagern

  • Status changed from needs_work to needs_review

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.

comment:20 Changed 7 years ago by jakobkroeker

  • Status changed from needs_review to needs_work

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.

comment:21 Changed 7 years ago by jakobkroeker

  • Branch changed from u/gagern/ticket/16570 to u/jakobkroeker/ticket/16570
  • Modified changed from 09/17/14 14:16:09 to 09/17/14 14:16:09

comment:22 Changed 7 years ago by jakobkroeker

  • Commit changed from a5e766dcccea56a5af130dc4f7cde649997dc646 to 9df42a472cd18d9f7b0eaa3cf820c515f74e54a6
  • Status changed from needs_work to needs_review

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:

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

comment:23 follow-up: Changed 7 years ago by 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.

comment:24 in reply to: ↑ 23 ; follow-up: Changed 7 years ago by 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.

comment:25 Changed 7 years ago by gagern

  • Status changed from needs_review to needs_work

comment:26 in reply to: ↑ 24 Changed 7 years ago by 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.

comment:27 Changed 7 years ago by gagern

  • Branch changed from u/jakobkroeker/ticket/16570 to u/gagern/ticket/16570
  • Modified changed from 10/08/14 14:00:26 to 10/08/14 14:00:26

comment:28 Changed 7 years ago by gagern

  • Commit changed from 9df42a472cd18d9f7b0eaa3cf820c515f74e54a6 to 07eaa4c4164dd734dccd6c14730bc70256efaf93
  • Status changed from needs_work to needs_review

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


New commits:

403a9e6animate: Pass keyword arguments to plot constructor.
9e4d39cMerge tag '6.4.beta4' into ticket/16570
e14ebdbMark some animation tests as long time.
17d4f90Minor improvements to aminate documentation.
07eaa4cImprove some more animation examples.

comment:29 Changed 7 years ago by git

  • Commit changed from 07eaa4c4164dd734dccd6c14730bc70256efaf93 to 9d9e590f5ef642a06f45056ac282fe696131c435

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

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

comment:30 follow-up: Changed 7 years ago by 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.)

comment:31 in reply to: ↑ 30 Changed 7 years ago by 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.

comment:32 follow-ups: Changed 7 years ago by jakobkroeker

  • Status changed from needs_review to needs_info

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

comment:33 in reply to: ↑ 32 Changed 7 years ago by 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.

comment:34 in reply to: ↑ 32 Changed 7 years ago by 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.

Last edited 7 years ago by kcrisman (previous) (diff)

comment:35 Changed 7 years ago by kcrisman

  • Reviewers changed from Jakob Kroeker to Jakob Kroeker, Karl-Dieter Crisman
  • Status changed from needs_info to positive_review

comment:36 Changed 7 years ago by 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 ?

comment:37 follow-up: Changed 7 years ago by ppurka

See this #13368

comment:38 in reply to: ↑ 37 Changed 6 years ago by 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.

comment:39 Changed 6 years ago by vbraun

  • Branch changed from u/gagern/ticket/16570 to 9d9e590f5ef642a06f45056ac282fe696131c435
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:40 Changed 6 years ago by kcrisman

  • Commit 9d9e590f5ef642a06f45056ac282fe696131c435 deleted

Possible bad outcome to this revealed in #18176.

Note: See TracTickets for help on using tickets.