image-processing icon indicating copy to clipboard operation
image-processing copied to clipboard

Blur Animation source code and simplified output

Open marcodallavecchia opened this issue 1 year ago • 1 comments

Hello everyone! I've seen from this issue, that we were looking to implement a better blur animation figure for section 06-blurring.

I've implemented a Python script that creates this animation with Matplotlib. Here are a few points:

  • I created a simple single-channel 30x30 pixels image of a letter A (using ImageJ) → available in episodes/data/A.tif
  • I exported the animation output directly in episodes/fig/blur-demo.gif
  • The animation has as a first frame the complete blurred image (for the print version, as described in the issue)
  • I chose more or less arbitrarily the colors → please let me know which colors you think could be better / more accessible
  • The code works but it's not the cleanest nor the most structured. But I tried to keep it well documented.
  • I've attached a small README and env.yml file for the requirements
  • Of course, with this script, anyone can change the source image, colors, speed and more of the animation and create a better version if desired.

A final extra point from my side: I'm a big believer that every output should be associated with its source-code, especially in the carpentries context where we are trying to keep things reproducible. In this case, I couldn't find the code used to create the previous version of the blur-demo.gif so I took the freedom to create a episodes/files/source-code/ directory, where we could pool together all the code snippets for generating all the output that would later feed on the website (this is where my current create_blur_animation.py file is). I am unsure if this is the best place to have such folder, maybe in the root could even be better for clear reference.

I hope this is helpful for the issue and course material. I am open to improve it in the near future after some feedback!

marcodallavecchia avatar Jun 20 '24 14:06 marcodallavecchia

Thank you!

Thank you for your pull request :smiley:

:robot: This automated message can help you check the rendered files in your submission for clarity. If you have any questions, please feel free to open an issue in {sandpaper}.

If you have files that automatically render output (e.g. R Markdown), then you should check for the following:

  • :dart: correct output
  • :framed_picture: correct figures
  • :question: new warnings
  • :bangbang: new errors

Rendered Changes

:mag: Inspect the changes: https://github.com/datacarpentry/image-processing/compare/md-outputs..md-outputs-PR-328

The following changes were observed in the rendered markdown documents:

 06-blurring.md                                     |   3 +-
 data/letterA.tif (new)                             | Bin 0 -> 1136 bytes
 fig/blur-demo.gif                                  | Bin 32633468 -> 567305 bytes
 .../06-blurring/create_blur_animation.py (new)     | 160 +++++++++++++++++++++
 md5sum.txt                                         |   2 +-
 5 files changed, 162 insertions(+), 3 deletions(-)
What does this mean?

If you have source files that require output and figures to be generated (e.g. R Markdown), then it is important to make sure the generated figures and output are reproducible.

This output provides a way for you to inspect the output in a diff-friendly manner so that it's easy to see the changes that occur due to new software versions or randomisation.

:stopwatch: Updated at 2025-03-25 20:09:35 +0000

github-actions[bot] avatar Jun 20 '24 14:06 github-actions[bot]

Thank you very much @marcodallavecchia for this contribution. We discussed this in today's Curriculum Advisory Committee meeting. We do like the new animation and envision that it can be included in the blurring episode.

Regarding the location of the script, we do not have an ideal solution but our consensus (as of now) is to keep code scripts that re-generate figures in episodes/fig/source/. The reasoning is that we do not want to scare or distract learners with more complex code than what is covered in the lesson, and we believe they are less likely to look into a subfolder of the fig/ directory than the files/ directory. I have taken the liberty to update the branch accordingly.

One suggestion I have is that you might use a default value for the kernel size so that maintainers can reproduce the figure without having to guess the kernel size. I tend to prefer scripts that do not require interactive input for reproducibility.

There may be other feedback on the script, so I'll keep the pull request open for others to comment - :wave: @mkcor.

uschille avatar Nov 28 '24 00:11 uschille

@uschille thanks a lot for coming back to this!

  • I can definitely add a default kernel size or even remove the user input completely for reproducibility purposes.
  • Regarding the organization, I agree with the folder structure you opted for, mine was just a proposition, because I couldn't find any code to create the previous animation
  • I will wait to make any changes to see if any other feedback reaches my ear any time soon

:wave: Marco

marcodallavecchia avatar Nov 28 '24 10:11 marcodallavecchia

Thanks @marcodallavecchia - the new blur animation looks great! I'd agree with @uschille 's comments above.

We will need to update the text in the blurring episode to match this new animation. For example, above this gif it says This animation shows how the blur kernel moves along in the original image in order to calculate the colour channel values for the blurred image. - so we'd need to do some re-wording to make it clear that it's grayscale etc. Potentially it would be good to replace all the early parts of the episode that reference cat images to use the grayscale A instead. @uschille - what's your preference for this? We could handle this in a separate PR, but then we'd need to keep the original blur-demo.gif around in the meantime.

K-Meech avatar Nov 30 '24 17:11 K-Meech

We will need to update the text in the blurring episode to match this new animation.

Yes, it will require a bit of work to incorporate the new animation in the lesson.

@marcodallavecchia would you perhaps be willing to give it a try?

I'd suggest to keep this PR open while we are working on it. Does this sound reasonable @K-Meech?

uschille avatar Nov 30 '24 21:11 uschille

Sounds good to me @uschille !

K-Meech avatar Dec 01 '24 16:12 K-Meech

Dear @uschille and @K-Meech, Thanks a lot for the feedback and having the foresight of the needed changes in the lecture itself. I would gladly take the challenge to try and adapt it, given some time (probably until after the holidays).

Just to clarify, should I open a new PR only with the lecture changes, or should I integrate the text changes with the blur animation code altogether?

marcodallavecchia avatar Dec 02 '24 08:12 marcodallavecchia

Great - thanks @marcodallavecchia ! I'd say incorporate it directly into this PR, along with the blur animation code.

K-Meech avatar Dec 03 '24 09:12 K-Meech

Hello, sorry for the long delay on an update on this! I just pushed an update on the text and code of the blurring animation for this PR. I ended up not replacing the cat picture for the A image, but simply adapting the text to make it consistent with what the animation shows. If you think replacing the cat picture example is still recommended, I can still do it in the new future, let me know what you think. Cheers, Marco

marcodallavecchia avatar Feb 15 '25 19:02 marcodallavecchia

Hello @mkcor! Thanks for the feedback again. I implemented all the changes you suggested, the only thing I'm not sure about would be about the package requirements. I would be in favor of adding a conda env.yml or a pip requirements.txt file in the source directory if we really want to make the script fully reproducible. Let me know if you like the idea and I can add one!

marcodallavecchia avatar Mar 15 '25 11:03 marcodallavecchia

Hello! I am so sorry I was trying to understand how updating the fork would work on my side of the repo and I seem to have accidentally merged the datacarpentry:main into the mdv/blur-anim branch which then is affecting this PR. Can you please someone guide me how to fix it? Sorry again!

marcodallavecchia avatar Mar 21 '25 05:03 marcodallavecchia

@marcodallavecchia it looks like your merge of main made no difference to the files involved in this PR so I think you have nothing to worry about ❤️

tobyhodges avatar Mar 21 '25 08:03 tobyhodges

Thank you @tobyhodges! Lucky for me, I didn't break anything! I just pushed another commit on this branch with the incorporation of the last comments. I leave to you if you consider this a good final solution for this PR. Thanks a lot for helping me throughout the process

marcodallavecchia avatar Mar 25 '25 20:03 marcodallavecchia

Thanks for persevering, @marcodallavecchia 🙌

@datacarpentry/image-processing-curriculum-maintainers this feels to me like it is ready, but since I am no longer one of the Maintainers of the lesson I leave it to one of you to decide whether or not the time has come to push the big green merge button...

tobyhodges avatar Mar 25 '25 20:03 tobyhodges

One more point that I have been mulling over this morning, @marcodallavecchia. You wrote

Thank you @tobyhodges! Lucky for me, I didn't break anything!

but I think that merging our main into your branch was fine to do -- perhaps even good! In this instance, there had been no changes in main to the files you have been working on. But in other circumstances there might have been. In those cases, it is possible that a merge conflict could arise between your version of the file(s) and what was now in main and a human would need to resolve those conflicts. The way to do this would be to merge in main and work through the conflicts.

Often, a project maintainer will be happy to do that but in my view it is good open source etiquette for the contributor to handle merge conflicts and ensure that their branch is ready to merge. So in my opinion, merging main was good practice.

tobyhodges avatar Mar 26 '25 08:03 tobyhodges

Hello again @tobyhodges. What a great remark, thanks a lot for the clear explanation. I completely see the point and, of course, I would have been happy to go through potential conflicts in the merge with main. Happy to learn from an almost mistake ;)

marcodallavecchia avatar Mar 26 '25 16:03 marcodallavecchia

So in my opinion, merging main was good practice.

I concur; GitHub's interface 'invites' you to do so with the "Update branch" button. It's equivalent (and, hence, equally good) if you do it locally at the command line and then push.

mkcor avatar Mar 26 '25 17:03 mkcor

Thanks all! I should have time this weekend to give this a final look over, and then hopefully we can merge :smile: :tada:

K-Meech avatar Mar 27 '25 14:03 K-Meech

Thanks @marcodallavecchia - I had another look through, and this is looking great! :smile: I'll go ahead and merge now - thanks for the contribution! Super nice to see this gif finally updated :tada:

K-Meech avatar Mar 28 '25 19:03 K-Meech

Hello again @K-Meech! Thanks a lot for guiding me through this process and letting me contribute to the project! I hope there will be many more in the future. Excited to see this done :confetti_ball:

marcodallavecchia avatar Mar 31 '25 16:03 marcodallavecchia