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

Transition to new import convention for sckit-image? #295

Closed
mkcor opened this issue Aug 16, 2023 · 4 comments · Fixed by #299
Closed

Transition to new import convention for sckit-image? #295

mkcor opened this issue Aug 16, 2023 · 4 comments · Fixed by #299
Labels
type:discussion Discussion or feedback about the lesson

Comments

@mkcor
Copy link
Contributor

mkcor commented Aug 16, 2023

How could the content be improved?

Hi all,

Since I just merged scikit-image/scikit-image#7024 by @lagru, I thought I should propagate here the recommendation that we use import skimage as ski throughout the lesson.

See: https://github.com/scikit-image/scikit-image/blob/main/CONTRIBUTING.rst#stylistic-guidelines

@tobyhodges
Copy link
Member

Thank you for taking time to keep the lesson up-to-date with the latest changes in scikit-image, @mkcor.

I am concerned that this would introduce inconsistency in the way we are handling imports and might increase cognitive load. In most cases, we are importing some submodule of skimage rather than the whole thing (by my count, 14 of 19 scikit-image-related imports in the lesson import skimage.<something>). Aliasing skimage to ski would not be possible in these cases, and I think it would be a bit confusing for learners to see ski used in some cases but skimage used elsewhere.

So the question perhaps becomes: do we want to switch over to importing all of skimage instead of only specific submodules? I think @datacarpentry/image-processing-curriculum-maintainers talked about this once before, though I cannot find the notes now, and concluded that we are modeling better practice by importing specific submodules as needed.

@mkcor
Copy link
Contributor Author

mkcor commented Aug 17, 2023

Hi @tobyhodges,

I think https://github.com/orgs/datacarpentry/teams/image-processing-curriculum-maintainers talked about this once before, though I cannot find the notes now, and concluded that we are modeling better practice by importing specific submodules as needed.

For educational materials, I'm also in favour of importing specific submodules. So let's forget about import skimage as ski. Do we want import skimage.filters or from skimage import filters though? I used to use the latter in scikit-image tutorials (e.g., from skimage import color, filters).

Currently, file episodes/06-blurring.md contains:

import skimage                                                                 
import skimage.filters

and

from skimage.filters import gaussian

elsewhere.

I'm happy to update #294 with import fixes if we agree on a convention for importing submodules. Thanks!

@tobyhodges
Copy link
Member

I believe we prefer the import skimage.filters form because it is more explicit, making it easier for learners to identify where a particular function or submodule has come from.

I should really expand our lesson's CONTRIBUTING.md to mention some of these stylistic things. E.g. also #291 and #288

@tobyhodges tobyhodges added the type:discussion Discussion or feedback about the lesson label Aug 17, 2023
@mkcor
Copy link
Contributor Author

mkcor commented Aug 24, 2023

Thank you, @tobyhodges! Done f477193.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:discussion Discussion or feedback about the lesson
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants