Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider refactoring I/O parts #211

Open
JCGoran opened this issue Sep 17, 2024 · 1 comment
Open

Consider refactoring I/O parts #211

JCGoran opened this issue Sep 17, 2024 · 1 comment
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@JCGoran
Copy link
Contributor

JCGoran commented Sep 17, 2024

Doing some benchmarks, I've noticed that the I/O parts are done in the main loop, which causes an increase in the total runtime.
As an example, we have:

time python -m data_morph --seed 42 --start-shape panda --target-shape star --iterations 10000
13.81s user 0.79s system 109% cpu 13.310 total

but if we remove this block (which is in the loop):

frame_number = record_frames(
data=morphed_data,
count=frame_numbers.count(i),
frame_number=frame_number,
)

we achieve a major speed-up:

time python -m data_morph --seed 42 --start-shape panda --target-shape star --iterations 10000
5.41s user 0.28s system 137% cpu 4.149 total

I'd propose that instead of doing the I/O in the main loop, the frames that would be written to disk are simply stored in some internal list, and then I/O is done after all the computations. This has several benefits:

  1. if keep_frames=False, we don't output anything other than the final GIF, so we spare the disk from unnecessary writes
  2. if keep_frames=True, since the task is probably I/O bound, we can take advantage of the concurrent.futures module to do it concurrently, so the speed-up is probably still significant
  3. less error-prone since we don't need to find the files. There's also no guarantee that the files won't change on disk while the sim is running
  4. the most obvious one, speed! If using keep_frames=False, which is the default, we just output one file instead of potentially hundreds.
@stefmolin
Copy link
Owner

I don't fully understand what you mean by having just one file because even if we move this logic outside of the main loop, the animation logic currently relies on stitching together the images, meaning they still need to be created. I also don't want to remove the ability to retrieve frames (images or CSVs).

What you plan to store in order to be able to create the animation later? Have you thought about the specifics?

I'm not opposed to this, but I think this needs to be fleshed out more before we proceed.

@stefmolin stefmolin added the enhancement New feature or request label Sep 23, 2024
@stefmolin stefmolin added this to the 0.4.0 milestone Sep 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants