#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: |
Description (last modified by )
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.
Attachments (1)
Change History (41)
Changed 8 years ago by
comment:1 Changed 8 years ago by
- Description modified (diff)
Including image in the ticket description.
comment:2 Changed 8 years ago by
- 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 8 years ago by
- Commit set to d5155020dc2931d166f360f044a08b7b7bb78b24
comment:4 Changed 8 years ago by
- Status changed from new to needs_review
comment:5 follow-up: ↓ 6 Changed 8 years ago by
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 8 years ago by
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 8 years ago by
- 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 8 years ago by
- Milestone changed from sage-6.3 to sage-6.4
comment:9 follow-up: ↓ 15 Changed 8 years ago by
- 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 8 years ago by
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 8 years ago by
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 8 years ago by
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.
comment:13 Changed 8 years ago by
comment:14 Changed 8 years ago by
- Commit changed from d5155020dc2931d166f360f044a08b7b7bb78b24 to 050c3b8afb736b20e46feb85f9273fba35336688
Branch pushed to git repo; I updated commit sha1. New commits:
050c3b8 | Merge tag '6.3' into ticket/16570
|
comment:15 in reply to: ↑ 9 Changed 8 years ago by
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 8 years ago by
- Status changed from needs_info to needs_review
comment:17 follow-up: ↓ 19 Changed 8 years ago by
- 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 8 years ago by
- Commit changed from 050c3b8afb736b20e46feb85f9273fba35336688 to a5e766dcccea56a5af130dc4f7cde649997dc646
Branch pushed to git repo; I updated commit sha1. New commits:
a5e766d | Improve some more animation examples.
|
comment:19 in reply to: ↑ 17 Changed 8 years ago by
- 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 8 years ago by
- 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 8 years ago by
- 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 8 years ago by
- 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:
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
|
comment:23 follow-up: ↓ 24 Changed 8 years ago by
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: ↓ 26 Changed 8 years ago by
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 8 years ago by
- Status changed from needs_review to needs_work
comment:26 in reply to: ↑ 24 Changed 8 years ago by
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 8 years ago by
- 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 8 years ago by
- 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:
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.
|
comment:29 Changed 8 years ago by
- Commit changed from 07eaa4c4164dd734dccd6c14730bc70256efaf93 to 9d9e590f5ef642a06f45056ac282fe696131c435
Branch pushed to git repo; I updated commit sha1. New commits:
9d9e590 | fixed max y-axis range to 4 for the x^2 example
|
comment:30 follow-up: ↓ 31 Changed 8 years ago by
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 8 years ago by
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: ↓ 33 ↓ 34 Changed 8 years ago by
- 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 8 years ago by
Replying to jakobkroeker:
but I guess, in
make_image()
**kwds
should also be passed tosave_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 8 years ago by
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 tosave_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.
comment:35 Changed 8 years ago by
- Reviewers changed from Jakob Kroeker to Jakob Kroeker, Karl-Dieter Crisman
- Status changed from needs_info to positive_review
comment:36 Changed 8 years ago by
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
andxmax
ignored for G_kwds? - there is
xmin
andxmax
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 forplot(), show()
sufficient and understandable for users ?
comment:37 follow-up: ↓ 38 Changed 8 years ago by
See this #13368
comment:38 in reply to: ↑ 37 Changed 8 years ago by
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 8 years ago by
- Branch changed from u/gagern/ticket/16570 to 9d9e590f5ef642a06f45056ac282fe696131c435
- Resolution set to fixed
- Status changed from positive_review to closed
comment:40 Changed 7 years ago by
- Commit 9d9e590f5ef642a06f45056ac282fe696131c435 deleted
Possible bad outcome to this revealed in #18176.
Current broken state of the example