Opened 5 years ago

Closed 5 years ago

#16571 closed enhancement (fixed)

APNG (Animated PNG) for animations

Reported by: gagern Owned by:
Priority: major Milestone: sage-6.4
Component: graphics Keywords:
Cc: niles Merged in:
Authors: Martin von Gagern Reviewers: Niles Johnson
Report Upstream: N/A Work issues:
Branch: b0c13b6 (Commits) Commit: b0c13b6cea64f59b9c170236115cd7d68654205b
Dependencies: #17571 Stopgaps:

Description (last modified by gagern)

Currently the creation of animations depends on external tools: either ImageMagick or FFmpeg must be available. Either of these can be used to assemble a sequence of PNG frames into a GIF animation. However, GIF is a very limited format, and the conversion (I guess mostly the color reduction) takes time and costs quality.

A more modern alternative would be APNG. It's an extension of the PNG standard. One drawback is that only some browsers support it. The others would show the first frame of the animation as still image instead. (We could later extend the code so one can specify any image of equal size to be used as fallback.)

Assembling multiple PNG files into a single animation is fairly simple. In fact so simple that it can be accomplished without using any external libraries at all. I wrote such an implementation while working on #16533 and would like to offer it here for inclusion.

The branch I'm going to append uses commits from #16570 and #16645, so if those get merged before, you will have less to review here.

Change History (43)

comment:1 Changed 5 years ago by gagern

  • Branch set to u/gagern/ticket/16571
  • Created changed from 06/27/14 18:38:36 to 06/27/14 18:38:36
  • Modified changed from 06/27/14 18:38:36 to 06/27/14 18:38:36

comment:2 Changed 5 years ago by gagern

  • Commit set to d09c960adc629ac0b6bf937ef04e10a88bbc2936
  • Status changed from new to needs_review

New commits:

ac83bdbProper hyperlink formatting in animate documentation.
031b97eBase Animation.show on Animation.save, and pass more arguments.
005b83fUnify file name generation for animations.
834a96dAdd APNG support.
f07e57dTurn generator into list when rendering frames.
04665b0Fix frame size for standard example animation.
d09c960Merge fix of standard example animation into APNG branch.

comment:3 Changed 5 years ago by gagern

sage -coverage complains about missing doctests in all methods of my APngAssembler class. But the doctest in the class documentation tests all the methos implicitely. And checking whether the result is actually correct would mean either looking at a generated picture yourself, or writing down long byte sequences of image file content. Is my current lack of doctests acceptable in this case?

comment:4 Changed 5 years ago by gagern

  • Status changed from needs_review to needs_work

comment:5 Changed 5 years ago by git

  • Commit changed from d09c960adc629ac0b6bf937ef04e10a88bbc2936 to f4a242733b301b813f316d8661485b77ad520ee7

Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:

16cbd2dMerge branch 'develop' into ticket/16533-stopgap
b3e656bgraphics_filename: return a tmp_filename() if not in EMBEDDED_MODE
69b285dRestore possibly positional arguments.
e3c5542Merge branch ticket/15515 into ticket/16608 to create ticket/16645.
9f30e94Always use graphics_filename instead of tmp_filename.
bb7307fPrevent doctest from leaking file into current working directory.
e2d3e11Proper hyperlink formatting in animate documentation.
bd17f04Add APNG support.
a60fe0bTurn generator into list when rendering frames.
f4a2427Merge fix of standard example animation into APNG branch.

comment:6 Changed 5 years ago by gagern

  • Dependencies changed from #16533, #16570 to #16570, #16645
  • Description modified (diff)
  • Status changed from needs_work to needs_review

Rebased onto #16645. There are a lot of commits from the dependencies listed above. Only three commits are excludive to this ticket here, so these are the changes that need to be reviewed in this ticket here.

At the moment, the APNG support isn't integrated with the save and show methods. I'd do so based on #7298, but in order to avoid dependency hell, I'll deal with this in a separate ticket so you can review the changes here independently.

comment:7 follow-up: Changed 5 years ago by niles

  • Cc niles added

I'm slightly nervous about the long-term stability of the apng format, but given how easy it is to implement and how nice the results are, I suppose this is a reasonable addition. Some questions/comments:

  • I would expect a method named APngAssembler.frame to return a frame. Would add_frame or append_frame or something similar be a better name for this?
  • Since the docstring of APngAssembler.__init__ won't appear in the manual, it might be more useful to move that information to the class docstring.
  • most of the APngAssembler methods are missing doctests.

comment:8 in reply to: ↑ 7 ; follow-up: Changed 5 years ago by gagern

Replying to niles:

I'm slightly nervous about the long-term stability of the apng format […]

I'm less worried about stability and more worried about market share. If one day MNG gets widely adopted, it might make APNG superfluous. But I don't see that coming soon.

  • I would expect a method named APngAssembler.frame to return a frame. Would add_frame or append_frame or something similar be a better name for this?

Fine with me. I will include it in my next commit. I guess that in the long run, I might also add a method which I'd have called fallback or still, which can be used to set the fallback image to something other than the first frame. Would you prefer set_still or add_fallback or something like this?

  • Since the docstring of APngAssembler.__init__ won't appear in the manual, it might be more useful to move that information to the class docstring.

Oh, I hadn't realized that. Sure, will move that.

  • most of the APngAssembler methods are missing doctests.

Mentioned that problem in comment:3. I can try to come up with some doctests if you insist, but they'd be pretty ugly to read.

By the way, should I prepend a _ to all my internal methods?

Quoting from the commit message of that commit: “The APNG pipeline requires the number of frames up front”. I guess I could also save the *number* of frames, instead of the frames themselves. Would you prefer that? It would even be consistent with the doctest.

An inferior alternative would be copying what graphics_array() does:

        frame_list = list(self._frames)
        n = len(frame_list)

There you also have to maintain the list of all frames in memory, although arguably only for the duration of the method call. What I consider even more harmful, however, is the fact that the frames will be generated multiple times. In my opinion, a generator should only be traversed once for the workflow from construction to APNG output, and that one occurrence should be when building the PNGs.

comment:9 in reply to: ↑ 8 ; follow-up: Changed 5 years ago by niles

Replying to gagern:

Replying to niles:

I guess that in the long run, I might also add a method which I'd have called fallback or still, which can be used to set the fallback image to something other than the first frame. Would you prefer set_still or add_fallback or something like this?

Let's use whatever term the APNG developers (Mozilla?) use for it . . .

  • most of the APngAssembler methods are missing doctests.

Mentioned that problem in comment:3. I can try to come up with some doctests if you insist, but they'd be pretty ugly to read.

well, we have to have doctests . . . even if humans don't read them, they're very useful in making sure that new code doesn't break old code. When writing them, think "What is a machine-checkable way to ensure that this method is producing what it claims to produce?" The current doctest for APngAssembler is probably overkill. Also note that, if necessary, you can use the #indirect doctest flag to mark doctests which check the relevant method implicitly, but not explicitly.

By the way, should I prepend a _ to all my internal methods?

It's sort of a grey area . . . the key difference is that underscore methods won't show up in the reference manual, for better or worse.

Quoting from the commit message of that commit: “The APNG pipeline requires the number of frames up front”.

I see. I haven't checked to see why that's necessary . . . are you sure it can't be avoided?

I guess I could also save the *number* of frames, instead of the frames themselves.

If you can do that without loading them all into memory, that would be great

An inferior alternative would be copying what graphics_array() does:

        frame_list = list(self._frames)
        n = len(frame_list)

There you also have to maintain the list of all frames in memory, although arguably only for the duration of the method call. What I consider even more harmful, however, is the fact that the frames will be generated multiple times.

right

In my opinion, a generator should only be traversed once for the workflow from construction to APNG output, and that one occurrence should be when building the PNGs.

I fully agree. Ideally, this is done one item at a time, rather than loading all frames into memory simultaneously.

comment:10 in reply to: ↑ 9 Changed 5 years ago by gagern

Replying to niles:

Replying to gagern:

I guess that in the long run, I might also add a method which I'd have called fallback or still, which can be used to set the fallback image to something other than the first frame. Would you prefer set_still or add_fallback or something like this?

Let's use whatever term the APNG developers (Mozilla?) use for it . . .

Mozilla terminology is talking about a “default image” which either is the first frame or “not part of the animation”. So I'm looking for a name to set a default image which is not part of the animation. set_default_image? My main concern here is the verb, since this sounds like a simple setter with no heavy lifting.

  • most of the APngAssembler methods are missing doctests.

Mentioned that problem in comment:3. I can try to come up with some doctests if you insist, but they'd be pretty ugly to read.

well, we have to have doctests . . . even if humans don't read them, they're very useful in making sure that new code doesn't break old code. When writing them, think "What is a machine-checkable way to ensure that this method is producing what it claims to produce?" The current doctest for APngAssembler is probably overkill. Also note that, if necessary, you can use the #indirect doctest flag to mark doctests which check the relevant method implicitly, but not explicitly.

I've started to write a doctest for each of my methods, see here for the current state of affairs. But I'm very unhappy about this. Most of the methods I consider private (and I added _ to them by now). Each of them doesn't do much on its own; it relies on the interplay with the other methods to make things work. So I have basically two options.

On the one hand I could see what internal variables each of the other methods would set up front, then reproduce this in the example, and test the result of a single method in response to that. But this fixes the internal mechanics of the whole class into the doctest, i.e. I'm testing how things get done, not only that they get done. This feels wrong.

On the other hand, I could write a doctest which uses high level calls, but whose output depends on the method I want to test. That's these indirect doctests you mentioned. But there would be a lot of boilerplate code for each method, and most things would be common for all tests. In the end, I'd have 20 lines of doctests for 3 lines of code, most of these similar for all methods in question.

So instead of one doctest per method, I'd like to have a single doctest which tests the whole machinery, designed in such a way that it will only work if all the parts interact as intended. In fact in the code I linked above there is such a test for the frame method. Can I omit the doctests for the individual methods if I have some doctest somewhere which tests them all? Should I somehow indicate that the test for these is somewhere else?

I've just posted to sage-devel to obtain some broader input on this question of doctest granularity.

There is one other problem with the doctests as they currently are: Sphinx is choking on them.

[plotting ] Sphinx error:
[plotting ] 'ascii' codec can't decode byte 0x89 in position 683: ordinal not in range(128)
Error building the documentation.

I have no idea why Sphinx is trying to decode a string literal here, but this might make doctests using binary data almost impossible. Which means that I'll likely have to build the data in question from some struct calls, or write a helper function to convert largish chunks of hex to binary, or whatever.

comment:11 Changed 5 years ago by git

  • Commit changed from f4a242733b301b813f316d8661485b77ad520ee7 to 941170b5ce91c89a92cd86f3f8eef698dd0b2e8e

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

bc9f681Merge fix of standard example animation into APNG branch.
780e614Proper hyperlink formatting in animate documentation.
10f8e52Make number of frames available after rendering using generator.
941170bAdd APNG support.

comment:12 Changed 5 years ago by gagern

I rebased this branch and reordered my commits. Now I can more easily amend the latest commit. And I can also provide a stable link to the changes which need reviewing here, based on the merge which is now the first commit not shared with some other ticket.

The core currently has two major tests for the APngAssembler class, one for each public method. These will test all the internal methods as well. I realized that my Sphinx problems were due to the fact that I started my docstring with """ not r""". But by that time I had my tests converted to make use of the h2b utility function, and I found that the file content looked far more readable in that format, so I kept it.

comment:13 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:14 Changed 5 years ago by git

  • Commit changed from 941170b5ce91c89a92cd86f3f8eef698dd0b2e8e to 3a9922592c1336f2a3e5e9774572a7f8f7450d97

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

3a99225Merge ticket/16645 into ticket/16571 to deal with conflict.

comment:15 Changed 5 years ago by git

  • Commit changed from 3a9922592c1336f2a3e5e9774572a7f8f7450d97 to 2daccc75010763e6f5a6cbabfea3fa202171a59f

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

e216b92Make number of frames available after rendering using generator.
2daccc7Add APNG support.

comment:16 follow-up: Changed 5 years ago by gagern

Rebased to handle changes from #16570. It would be really nice if we could get this merged one day soon.

In case the way I implemented doctests for my public methods, and avoided doctests for my internal methods, is still causing worries, I'd like some specific input on how to improve things. Yes, I've heard that every method should have a doctest, that there should be as few exceptions as possible, but I still honestly fail to see how I can reasonably do so for the case at hand. At the moment I have class-level documentation for the APngAssembler class, which I'm fully comfortable with. I also have two public methods, add_frame and set_default which are tested using a hex representation of binary file content. A but bulky, but perhaps appropriate. If you want me to, I can move the h2b function which turns hex to binary from the doctest to the code proper.

And I have a bunch of internal methods which each do very little but which interact with one another in a tight net. They converse not only using parameters and return values but also member variables, so state plays a major role and setting up state would make doctests pretty big. All of these methods are tested implicitely by the tests for the public methods. No user is ever intended to call any of them by itself. Their interplay behind the scenes may change at any time, as long as the end result remains the same. Therefore I see no use at all in doctesting them. Documenting yes, but not testing.

If you can agree with the above approach, I'd welcome a positive review. If not, please tell me not only that this violates sage standards, but be more specific in what kind of doctest you would like to see added to what code location.

comment:17 in reply to: ↑ 16 ; follow-up: Changed 5 years ago by niles

Replying to gagern:

Rebased to handle changes from #16570. It would be really nice if we could get this merged one day soon.

Agreed.

In case the way I implemented doctests for my public methods, and avoided doctests for my internal methods, is still causing worries, I'd like some specific input on how to improve things.

Specific input: Write a doctest for every method. The feedback from this sage-devel thread was uniform on this point, and I think it would be unwise to ignore it.

Yes, I've heard that every method should have a doctest, that there should be as few exceptions as possible, but I still honestly fail to see how I can reasonably do so for the case at hand. . . . so state plays a major role and setting up state would make doctests pretty big.

Can you write a function which sets up a test state, or load a pickled test state? I believe this is what Volker was indicating when he mentioned fixtures.

All of these methods are tested implicitely by the tests for the public methods.

And if one of them breaks, and you're not around to fix it, how will someone figure out what's broken?

No user is ever intended to call any of them by itself.

Unfortunately your intentions don't control what others may do, either now or in the future. The requirement that all methods be tested is important because none of us can control, or even reliably predict, what will happen in the future.

comment:18 Changed 5 years ago by git

  • Commit changed from 2daccc75010763e6f5a6cbabfea3fa202171a59f to 1da7fb8f67f2ba95a1dd9166ea75c969e725bc01

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

1da7fb8Doctests for each method of APngAssembler; introducing tracing fixture.

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

Replying to niles:

Can you write a function which sets up a test state, or load a pickled test state? I believe this is what Volker was indicating when he mentioned fixtures.

OK, now I have test data in a class method, and can run a test case using that test data without including all the binary data in the doctest text. What is even more important, I can see the interaction of a given method with the object state using a tracing fixture I newly introduced for this purpose.

All of these methods are tested implicitely by the tests for the public methods.

And if one of them breaks, and you're not around to fix it, how will someone figure out what's broken?

The same way the internal workings behind any other public interface are fixed: by reading the docstring of the public interface, by examining the failing doctest of the public interface if there is any, by reading the comments (and in case of internal methods even docstrings) of the internal code, and by understanding the portion of internal code relevant to the problem. There is nothing magic about method boundaries. So as long as you don't require doctests for each and every line of code, I see no reason to doctest internal methods. As it stands, nested functions are usually without doctests, so should I use nested functions instead of classes with private methods? Or should we force everyone to doctest internal functions as well? Perhaps lambdas, too?

No user is ever intended to call any of them by itself.

Unfortunately your intentions don't control what others may do, either now or in the future. The requirement that all methods be tested is important because none of us can control, or even reliably predict, what will happen in the future.

Sorry, but users using code which is clearly designated internal are on their own. You can't take an arbitrary Sage object, mess directly with its internal state and hope for the result to behave in any reasonable way. Why would internal methods be any different?

If a sage developer at some point realizes that code could be reused by making a private method public, then that method should get doctested as well. But the same again holds for any piece of code: if some developer finds that a certain portion of an existing method could be reused, he's welcome to factor that out into a new public method and add doctests for it. Again I see no difference here, except that by adding doctests to internal methods I suggest that I might be willing to guarantee certain internal behavior in the future. Which I'm not, as long as the public interface remains.

Sorry for the rant. I'm still far from happy with this strict requirement. But since you are right and the mailing list feedback confirmed that requirement, I'm trying to make the best of it, and here it is. Now every method has a doctest, and every doctest shows the tested method in action. Can you give this a positive review?

comment:20 follow-up: Changed 5 years ago by kcrisman

I haven't had a chance to look at this, being unfamiliar with the tools... Just wanted to point out this and this; will anyone ever support this?

comment:21 follow-up: Changed 5 years ago by kcrisman

Now I have looked at it and realize it would take me far longer to review than reasonable... sorry. I *can* say that the basic relation to the rest of animate seems fine, it's the actual apng assembler I won't be reviewing. That said, I have a question about your docs. I see why you changed the first one from animate([function for el in list]) to animate([plot(function, (x,0,2)) for el in list]), but you made a lot more changes like that in the next patch which seemed gratuitous and which might serve to lead people away from this useful syntax.

Unless "explicit is better than implicit" in this case and we are deprecating that syntax, but then we should say so, and do it somewhere else. Or did all of those cause errors in generating the files?

comment:22 in reply to: ↑ 20 Changed 5 years ago by gagern

Replying to kcrisman:

I haven't had a chance to look at this, being unfamiliar with the tools... Just wanted to point out this and this; will anyone ever support this?

As per this comparison (which I also referred to in my documentation), Firefox and Safari do support it a the moment, amounting to over 17% market share. Less than I would wish for, but in my opinion still enough to warrant this effort. Particularly since GIF is so very limited in colors. For the rest of the world, it would be nice to offer <video> tags as an alternative (#7298) and perhaps try building an spkg for ffmpeg. But that's a different issue.

comment:23 in reply to: ↑ 21 Changed 5 years ago by gagern

  • Status changed from needs_review to needs_work

Replying to kcrisman:

Now I have looked at it and realize it would take me far longer to review than reasonable... sorry. I can say that the basic relation to the rest of animate seems fine, it's the actual apng assembler I won't be reviewing.

Sad but quite understandable. Thanks for the partial thumbs up on the integration!

That said, I have a question about your docs. I see why you changed the first one from animate([function for el in list]) to animate([plot(function, (x,0,2)) for el in list]), but you made a lot more changes like that in the next patch which seemed gratuitous and which might serve to lead people away from this useful syntax.

Sounds like some things originally aimed for #16570 (and superseded by the fixed regression there) somehow ended up in here during rebasing. I'll make an effort to disentangle that. Thanks for pointing this out.

comment:24 Changed 5 years ago by git

  • Commit changed from 1da7fb8f67f2ba95a1dd9166ea75c969e725bc01 to 4c7d5f209c1b443680bf4343581aa8e8885bf114

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

b057e61Add APNG support.
4c7d5f2Doctests for each method of APngAssembler; introducing tracing fixture.

comment:25 Changed 5 years ago by gagern

  • Status changed from needs_work to needs_review

Included fixup to avoid unneccessary modification of examples.

comment:26 follow-up: Changed 5 years ago by niles

  • Status changed from needs_review to needs_work

This is looking good to me: tests are good, functionality is good, code is clean and straightforward. HTML documentation looks fine too. I'm running automated doctests now, but don't expect any problems.

The only thing preventing me from positive review is that the tracer stuff really belongs on a separate ticket. When that's split into a separate ticket, I'll give them both positive reviews (modulo issues with the tracer stuff, below). As a very minor point, I think it would be useful if you put the date and a short description of your contribution in the AUTHORS block. Obviously this isn't always done, and isn't necessary, but I saw it recommended at some point and I think it's helpful. If you don't want to do it, that's fine.

Comments on fixtures.py:

  • The function traceMethod should be trace_method to fit the standard naming convention. Was there a strong reason for the name you gave it? If not, please change.
  • PropertyAccessTracerProxy.__init__ and PropertyAccessTracerProxy.fmt need docstrings
  • An author block and a copyright block are necessary at the top of the file. See here.
  • While you're at it, why not expand the documentation? This could be useful much more generally, and a very short description at the top of the document would help explain what it does.
  • It doesn't make sense to leave fixtures.py out of the reference manual. I think you add it by adding a line in src/doc/en/reference/doctest/index.rst, but I may be misremembering.

comment:27 Changed 5 years ago by niles

There are two tests in doctest/control.py that need to be updated because of the new fixtures file. Otherwise all tests pass.

comment:28 Changed 5 years ago by gagern

  • Dependencies changed from #16570, #16645 to #17571

Working on it.

comment:29 Changed 5 years ago by git

  • Commit changed from 4c7d5f209c1b443680bf4343581aa8e8885bf114 to f347d4ccaba191be973f241a0a6bf7f1f0a86fa5

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

f347d4cDoctests for each method of APngAssembler using tracing fixture.

comment:30 Changed 5 years ago by git

  • Commit changed from f347d4ccaba191be973f241a0a6bf7f1f0a86fa5 to 7d2531d65b2a86a1506dab9c141c65aca6f3c751

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

7d2531dDetails on my APNG contribution in module documentation.

comment:31 in reply to: ↑ 26 Changed 5 years ago by gagern

  • Status changed from needs_work to needs_review

Replying to niles:

The only thing preventing me from positive review is that the tracer stuff really belongs on a separate ticket.

OK, the code is separated now. The code presented here does depend on that of #17571 (nice that these two tickets are exactly 1000 apart), but I didn't merge that branch into this one here, so this one here, all by itself, won't pass doctests. You have to (temporarily) merge the two to get the whole picture. The stated dependencies of this ticket should ensure that things get merged into develop in the proper order.

When that's split into a separate ticket, I'll give them both positive reviews (modulo issues with the tracer stuff, below).

That would be really great!

comment:32 Changed 5 years ago by niles

  • Status changed from needs_review to positive_review

ok, looks good!

comment:33 follow-up: Changed 5 years ago by gagern

  • Reviewers set to ​Niles Johnson

Thanks for the review. Hope you don't mind me setting the Reviewer field for you.

The next step along the path of APNG integration would be #16650 which depends on #7298. If you feel like continuing, reviews for these would of course be highly appreciated. But in any case I'm glad for the reviews you already did, so don't feel forced to do more than you want to.

comment:34 in reply to: ↑ 33 Changed 5 years ago by niles

Replying to gagern:

Thanks for the review. Hope you don't mind me setting the Reviewer field for you.

Thanks! I should have done that.

The next step along the path of APNG integration would be #16650 which depends on #7298. If you feel like continuing, reviews for these would of course be highly appreciated. But in any case I'm glad for the reviews you already did, so don't feel forced to do more than you want to.

I'll take a look, but probably slowly.

comment:35 Changed 5 years ago by vbraun

  • Reviewers changed from ​Niles Johnson to Niles Johnson

comment:36 follow-up: Changed 5 years ago by vbraun

  • Status changed from positive_review to needs_work

Doctest failures in src/sage/plot/animate.py

comment:37 in reply to: ↑ 36 Changed 5 years ago by gagern

Replying to vbraun:

Doctest failures in src/sage/plot/animate.py

Did you merge #17571 along with this branch here? Do I need to merge it explicitely?

comment:38 Changed 5 years ago by vbraun

I did merge #17571. I don't merge tickets whose dependencies are not merged.

comment:39 Changed 5 years ago by vbraun

sage -t --long --warn-long 45.5 src/sage/plot/animate.py
**********************************************************************
File "src/sage/plot/animate.py", line 1122, in sage.plot.animate.APngAssembler._add_png
Failed example:
    APngAssembler._testCase1("_add_png", reads=False)
Exception raised:
    Traceback (most recent call last):
      File "/mnt/disk/home/release/Sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 488, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/mnt/disk/home/release/Sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 850, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.plot.animate.APngAssembler._add_png[1]>", line 1, in <module>
        APngAssembler._testCase1("_add_png", reads=False)
      File "/mnt/disk/home/release/Sage/local/lib/python2.7/site-packages/sage/plot/animate.py", line 1501, in _testCase1
        delay=0x567, delay_denominator=0x1234)
      File "/mnt/disk/home/release/Sage/local/lib/python2.7/site-packages/sage/plot/animate.py", line 1069, in add_frame
        self._add_png(pngfile)
      File "/mnt/disk/home/release/Sage/local/lib/python2.7/site-packages/sage/doctest/fixtures.py", line 368, in g
        arglst = [fmt(arg) for arg in args]
    NameError: global name 'fmt' is not defined
**********************************************************************
File "src/sage/plot/animate.py", line 1202, in sage.plot.animate.APngAssembler._first_IHDR
Failed example:
    APngAssembler._testCase1("_first_IHDR")
Exception raised:
    Traceback (most recent call last):
      File "/mnt/disk/home/release/Sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 488, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/mnt/disk/home/release/Sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 850, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.plot.animate.APngAssembler._first_IHDR[1]>", line 1, in <module>
        APngAssembler._testCase1("_first_IHDR")
      File "/mnt/disk/home/release/Sage/local/lib/python2.7/site-packages/sage/plot/animate.py", line 1501, in _testCase1
        delay=0x567, delay_denominator=0x1234)
      File "/mnt/disk/home/release/Sage/local/lib/python2.7/site-packages/sage/plot/animate.py", line 1069, in add_frame
        self._add_png(pngfile)
      File "/mnt/disk/home/release/Sage/local/lib/python2.7/site-packages/sage/plot/animate.py", line 1172, in _add_png
        met(cdata)
      File "/mnt/disk/home/release/Sage/local/lib/python2.7/site-packages/sage/doctest/fixtures.py", line 368, in g
        arglst = [fmt(arg) for arg in args]
    NameError: global name 'fmt' is not defined
**********************************************************************
File "src/sage/plot/animate.py", line 1219, in sage.plot.animate.APngAssembler._first_IDAT
Failed example:
    APngAssembler._testCase1("_first_IDAT")
Exception raised:
    Traceback (most recent call last):
      File "/mnt/disk/home/release/Sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 488, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/mnt/disk/home/release/Sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 850, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.plot.animate.APngAssembler._first_IDAT[1]>", line 1, in <module>
        APngAssembler._testCase1("_first_IDAT")
      File "/mnt/disk/home/release/Sage/local/lib/python2.7/site-packages/sage/plot/animate.py", line 1501, in _testCase1
        delay=0x567, delay_denominator=0x1234)
      File "/mnt/disk/home/release/Sage/local/lib/python2.7/site-packages/sage/plot/animate.py", line 1069, in add_frame
        self._add_png(pngfile)
      File "/mnt/disk/home/release/Sage/local/lib/python2.7/site-packages/sage/plot/animate.py", line 1172, in _add_png
        met(cdata)
      File "/mnt/disk/home/release/Sage/local/lib/python2.7/site-packages/sage/doctest/fixtures.py", line 368, in g
        arglst = [fmt(arg) for arg in args]
    NameError: global name 'fmt' is not defined
**********************************************************************
File "src/sage/plot/animate.py", line 1237, in sage.plot.animate.APngAssembler._next_IDAT
Failed example:
    APngAssembler._testCase1("_next_IDAT")
Exception raised:
    Traceback (most recent call last):
      File "/mnt/disk/home/release/Sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 488, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/mnt/disk/home/release/Sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 850, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.plot.animate.APngAssembler._next_IDAT[1]>", line 1, in <module>
        APngAssembler._testCase1("_next_IDAT")
      File "/mnt/disk/home/release/Sage/local/lib/python2.7/site-packages/sage/plot/animate.py", line 1502, in _testCase1
        apng.add_frame(cls._testData("input2", True))
      File "/mnt/disk/home/release/Sage/local/lib/python2.7/site-packages/sage/plot/animate.py", line 1069, in add_frame
        self._add_png(pngfile)
      File "/mnt/disk/home/release/Sage/local/lib/python2.7/site-packages/sage/plot/animate.py", line 1172, in _add_png
        met(cdata)
      File "/mnt/disk/home/release/Sage/local/lib/python2.7/site-packages/sage/doctest/fixtures.py", line 368, in g
        arglst = [fmt(arg) for arg in args]
    NameError: global name 'fmt' is not defined
**********************************************************************

comment:40 Changed 5 years ago by git

  • Commit changed from 7d2531d65b2a86a1506dab9c141c65aca6f3c751 to b0c13b6cea64f59b9c170236115cd7d68654205b

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

771948aAddress issues from first review.
2cbaa01Split property access tracing into proxy and helper classes.
f633454Improve documentation style.
8afc553Turn reproducible_repr from a class method to a standalone function.
b89fb60Use term attribute instead of property.
cb13887Fix trivial spelling mistake.
36241e3Clearer documentation for reproducible_repr.
e3093edAdd reproducible_repr for list and dict.
95e3f64Merge ticket/17571 into ticket/16571 since it's a dependency.
b0c13b6Fix formatting of arguments in trace_method wrapper.

comment:41 follow-up: Changed 5 years ago by gagern

  • Status changed from needs_work to needs_review

It's in fact a bug in code from #17571 where I hadn't covered all aspects in the doctests. Do I need a new review just for that last rename, or can you merge it as is?

comment:42 in reply to: ↑ 41 Changed 5 years ago by niles

  • Status changed from needs_review to positive_review

Replying to gagern:

It's in fact a bug in code from #17571 where I hadn't covered all aspects in the doctests. Do I need a new review just for that last rename, or can you merge it as is?

The error is just caused by a function renaming on #17571 (which was not caught because doctesting was insufficient.) The latest commit fixes the error (use correct function name) and improves an existing doctest to check it.

To summarize: a trivial bug in fixtures.py (code from #17571) was caught and fixed in the course of addressing this ticket.

I'm happy to give it positive review.

comment:43 Changed 5 years ago by vbraun

  • Branch changed from u/gagern/ticket/16571 to b0c13b6cea64f59b9c170236115cd7d68654205b
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.