Skip to content

Replace skimage.io with imageio.v3 for reading/writing images.#238

Merged
tobyhodges merged 23 commits intodatacarpentry:gh-pagesfrom
mkcor:use-imageio
Nov 21, 2022
Merged

Replace skimage.io with imageio.v3 for reading/writing images.#238
tobyhodges merged 23 commits intodatacarpentry:gh-pagesfrom
mkcor:use-imageio

Conversation

@mkcor
Copy link
Copy Markdown
Contributor

@mkcor mkcor commented Nov 5, 2022

Dear lesson maintainers,

Please let me know if I should have created an issue first, in order to discuss the change I'm submitting here. I wish to 'change this lesson' and the link given at https://github.com/datacarpentry/image-processing/blob/gh-pages/CONTRIBUTING.md#where-to-contribute leads to a 404, so I wasn't sure on how to proceed... I'm happy to have all the necessary discussions here, and I won't be offended at all if this PR gets closed (instead of merged).

I realize I should have committed the non-controversial changes separately, so they can get merged! I'm happy to re-submit them in a separate PR.

Getting to the gist of this PR, I'm following up on scikit-image/scikit-image#5036 (comment): The vision for scikit-image is to focus on actual image processing, delegating other functionalities (e.g., input/output, drawing, ...) to dedicated libraries. For reading and writing image data, this dedicated library is now available and stable, it is imageio.

Therefore, I'm thinking that, before releasing this lesson as 'stable,' we may want to consider making this change. If you agree with it, I'll propagate it to all subsequent episodes (episode 03 representing the biggest chunk). From an educational perspective, I can see how we may prefer sticking with only one library instead of two. I would argue that the proposed change doesn't add a dependency (you can readily import imageio once you have installed scikit-image); so the setup wouldn't be affected.

I could add a sentence to introduce the imageio library, if you deem it appropriate; the existing sentence seems almost sufficient and works well with the proposed change: "Let's load our image data from disk using the imread function from the imageio.v3 library and display it using ..."

Let me ping other scikit-image maintainers in case they might have different and/or additional points to make: @stefanv @jni @alexdesiqueira @grlee77 @lagru

Instructions

Thanks for contributing! ❤️

If this contribution is for instructor training, please email the link to this contribution to
checkout@carpentries.org so we can record your progress. You've completed your contribution
step for instructor checkout by submitting this contribution!

Keep in mind that lesson maintainers are volunteers and it may take them some time to
respond to your contribution. Although not all contributions can be incorporated into the lesson
materials, we appreciate your time and effort to improve the curriculum. If you have any questions
about the lesson maintenance process or would like to volunteer your time as a contribution
reviewer, please contact The Carpentries Team at team@carpentries.org.

You may delete these instructions from your comment.

- The Carpentries

@tobyhodges
Copy link
Copy Markdown
Member

Hi @mkcor thank you for the PR. I will review the specific changes in a moment, but first some responses to your introductory text.

Please let me know if I should have created an issue first, in order to discuss the change I'm submitting here.

It is ok to open the pull request without an associated issue but, given that the topic of switching skimage.io for imageio warrants some discussion, it might be better to address that in a new issue. I will open one, and leave the PR unmerged for now.

I wish to 'change this lesson' and the link given at https://github.com/datacarpentry/image-processing/blob/gh-pages/CONTRIBUTING.md#where-to-contribute leads to a 404, so I wasn't sure on how to proceed...

Sorry about that, and thanks for mentioning it 😬 I have now updated CONTRIBUTING.md and the links should be correct for anyone who comes here looking for the same information you did.

Copy link
Copy Markdown
Member

@tobyhodges tobyhodges left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @mkcor 🙌 The proofreading edits are all great, and I also like the changes you are suggesting to switch out skimage.io for imageio if we decide to go ahead with that. However, I will leave this unmerged for now while discussion continues about the skimage.io/imageio change in #239.

@tobyhodges
Copy link
Copy Markdown
Member

As mentioned in #239, please go ahead and make the relevant changes in this branch to replace skimage.io with imageio.v3 in the rest of the lesson @mkcor. It's going to end up being quite a big one, and in future it will be better to keep proofreading edits and substantial changes to the content in separate pull requests, but let's start from where we are in this case.

When you come to make the other changes, please adjust the title of this pull request to reflect that it is now much more about the adoption of imageio than it is about proofreading - that should make it easier for others to understand when they're browsing through the history of changes to the curriculum in future.

Thank you for driving this update - it is great to know that the lesson will be aligned with recommended practices for the scikit-image community.

@mkcor mkcor changed the title Proofread episode 02 and use imageio for reading/writing images. [WIP] Replace skimage.io with imageio.v3 for reading/writing images. Nov 9, 2022
mkcor and others added 2 commits November 9, 2022 19:57
@tobyhodges
Copy link
Copy Markdown
Member

In #239, @mkcor wrote:

[snip]

please add a sentence explaining that imageio.v3 is specifying that we want to use version 3 of the imageio API,

Done 726676f.

and another sentence describing the recommended division of tasks between imageio and skimage

Done 1f330e2 in the form of "Why not use skimage.io.imread()" -- I have drawn inspiration from episode 03 ("Why not use skimage.io.imshow()"). I have also added this recommendation as an objective e1ff043: Please double-check that this is fine.

I have updated the section about metadata aa584e8, since imageio.v3 does handle metadata (and nicely so! 🎉), which is yet another reason for using imageio when it comes to reading/writing images.

I have kept the warning because I noticed that imageio.v3 does not preserve all metadata. [snip]

Copying most of the reply here, to make the pull request thread easier for people to parse if they want to read through it in the future.

Copy link
Copy Markdown
Member

@tobyhodges tobyhodges left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only have one change to suggest, but otherwise this looks great. Thank you @mkcor.

As well as the setup file and the episodes, there is one "bonus episode" that we have been keeping in the _extras folder, about Edge Detection. Please could you also propagate the switch from skimage.io to imageio.v3 into that file?

@tobyhodges
Copy link
Copy Markdown
Member

tobyhodges commented Nov 15, 2022

@uschille the future of that extra Edge Detection episode is something that the Curriculum Advisory Committee could discuss at some stage, but I do not think it is urgent. I will open an issue and tag the CAC there.

I also noticed that you wanted to discuss this transition from skimage.io to imageio at the next CAC meeting. Would you like to wait to merge this PR until the committee has had a chance to discuss it?

@mkcor
Copy link
Copy Markdown
Contributor Author

mkcor commented Nov 15, 2022

Thank you, @tobyhodges! Sure, I still need to update episode 03 as well.

mkcor and others added 2 commits November 16, 2022 20:30
@mkcor
Copy link
Copy Markdown
Contributor Author

mkcor commented Nov 16, 2022

@mkcor
Copy link
Copy Markdown
Contributor Author

mkcor commented Nov 17, 2022

I haven't double-checked all the changes I've made in 5692fe8 (and I admit I'm not very motivated to do so in light of #241).

@mkcor mkcor changed the title [WIP] Replace skimage.io with imageio.v3 for reading/writing images. Replace skimage.io with imageio.v3 for reading/writing images. Nov 18, 2022
@mkcor
Copy link
Copy Markdown
Contributor Author

mkcor commented Nov 18, 2022

Dear lesson maintainers,

This PR is now ready for review! We will discuss it later today at the first CAC (Curriculum Advisory Committee) meeting.

@tobyhodges it looks like I cannot mention @curriculum-advisors-image... Could I have this permission, please? Thanks.

@tobyhodges
Copy link
Copy Markdown
Member

@tobyhodges it looks like I cannot mention @curriculum-advisors-image... Could I have this permission, please? Thanks.

I am surprised if you cannot mention the team, as you are a member. The team name in your post is correct, but missing the organisation name as a prefix - please could you double-check if you are able to tag the CAC using the full team name (at datacarpentry slash curriculum-advisors-image)?

@uschille uschille self-requested a review November 18, 2022 13:19
@mkcor
Copy link
Copy Markdown
Contributor Author

mkcor commented Nov 18, 2022

@datacarpentry/curriculum-advisors-image (see above) -- found it! Sorry for the noise, @tobyhodges 🙏

Copy link
Copy Markdown
Contributor

@uschille uschille left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great to me. Thanks @mkcor for updating all the episodes! I have only added two small edits to make the lesson template parser happy.

Co-authored-by: Ulf Schiller <uschille@users.noreply.github.com>
@mkcor
Copy link
Copy Markdown
Contributor Author

mkcor commented Nov 19, 2022

Thanks for reviewing, @uschille! :shipit:

@uschille
Copy link
Copy Markdown
Contributor

@tobyhodges The curriculum advisory committee is in favor of adopting imageio in this lesson. Please feel free to go ahead and merge this PR (it's currently blocked by requested changes). Thank you!

@tobyhodges
Copy link
Copy Markdown
Member

Thank you for this excellent effort, @mkcor. Merging now 🙌

@tobyhodges tobyhodges merged commit 6a9c8de into datacarpentry:gh-pages Nov 21, 2022
@uschille uschille linked an issue Nov 21, 2022 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

use imageio for input/output tasks?

3 participants