Modified class for additional functionality

[maojrs]

Hi Jake,

It seems I was using an old version of your JSAnimation, so I didn't have some of your final adjustments. I can try to add the changes to your newer version and do a new pull request (as i should have done from the beginning), though you might be able to merge them without confilcts.

The changes I made were: frame number box addition in animation windows, new function to generate general filenames, frame directory control, animation speed control, frame width control and option for additional html code in the preamble.

I don't know if all these changes are relevant, but they were very useful in order to implement it into Clawpack (also I like the new framebox). I made all the changes carefully to maintain ipython notebook compatibility.

Let me know what you think,
Mauricio

[jakevdp]

Hi - I've been traveling this week, so I haven't had a chance to look at this. I'll try to take a look next week

[jakevdp]

All of tabs should be four spaces -- it can cause problems to mix indentation levels like this.

[jakevdp]

Hi,
Something strange is going on with this PR -- the diff is really hard to read. It's hard to tell what you're trying to do, because there's a mix of old and new code. That's probably my fault for pushing some changes to master after we chatted. Still I like the idea of making frame names more configurable.

Can you try working from the current master, making sure to use four tabs, so that I can see what your changes are? Thanks

Oh - and sorry for taking so long to look at this! I really do appreciate the work you're doing here.

[maojrs]

Hi Jake

Thanks for all your input! I'll fork the latest version and try to add the changes there. I'll send you another pull request when it's ready.

Cheers,
Mauricio

On Aug 5, 2013, at 10:46 AM, Jake Vanderplas [email protected] wrote:

Hi,
Something strange is going on with this PR -- the diff is really hard to read. It's hard to tell what you're trying to do, because there's a mix of old and new code. That's probably my fault for pushing some changes to master after we chatted. Still I like the idea of making frame names more configurable.

Can you try working from the current master, making sure to use four tabs, so that I can see what your changes are? Thanks

Oh - and sorry for taking so long to look at this! I really do appreciate the work you're doing here.

—
Reply to this email directly or view it on GitHub.

[maojrs]

Hi Jake, I made the changes directly into your updated version. The PR should be easier to understand now. Let me know what you think.

[jakevdp]

I'd prefer not to hard-code the frame width here. If we create a figure with a given size in matplotlib, this size should be reflected in the resulting animation.

[jakevdp]

Lots of nitpicks -- sorry about that! In general it looks pretty good. I'd be happier if functions and variables were named in a way that didn't mislead the reader -- more detailed comments above.

[maojrs]

Hi Jake,

Thanks for your comments! I did apply some of your recommendations, and that's the version that
is already in Clawpack 5.0. Some of them I don't see a simple way to modify them to keep everyone
happy, so we might just keep a different version. If you are interested in some of the things that I added, but
you think it requires a bit more work, I'll be happy to do it. Otherwise, I might stop working on this for now, 
since it's already working smoothly in Clawpack.

Cheers,
Mauricio

---

From: Jake Vanderplas [email protected]
To: jakevdp/JSAnimation [email protected]
Cc: maojrs [email protected]
Sent: Tuesday, August 13, 2013 3:08 PM
Subject: Re: [JSAnimation] Modified class for additional functionality (#4)

Lots of nitpicks -- sorry about that! In general it looks pretty good. I'd be happier if functions and variables were named in a way that didn't mislead the reader -- more detailed comments above.
—
Reply to this email directly or view it on GitHub.