Thank you!

Thank you for your pull request 😃

🤖 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:

• 🎯 correct output
• 🖼️ correct figures
• ❓ new warnings
• ‼️ new errors

Rendered Changes

🔍 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:

 data/letterA.tif (new)                             | Bin 0 -> 1136 bytes
 fig/blur-demo.gif                                  | Bin 32633468 -> 567305 bytes
 .../06-blurring/create_blur_animation.py (new)     | 164 +++++++++++++++++++++
 3 files changed, 164 insertions(+)

⏱️ Updated at 2024-11-28 00:01:59 +0000

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 - 👋 @mkcor.

@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

👋 Marco

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.

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?

Sounds good to me @uschille !

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?

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