Connected components suggestions #316

JeremyPike · 2023-12-13T06:54:51Z

Suggestions and minor corrections for connected components episode. The most notable change is in the "Color mappings spoiler" where it looks like the default behaviour of matplotlib.pyplot.imshow has changed since the section was written such the data range for display is now automatically adjusted to the range of the image. The default colour map is also not grayscale so a empty image will appear blue not black I've modified the text to reflect these changes and also highlighted what vmin and vmax do which is very useful here.

github-actions · 2023-12-13T06:55:04Z

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-316

The following changes were observed in the rendered markdown documents:

 08-connected-components.md | 53 ++++++++++++++++++++++++++--------------------
 md5sum.txt                 |  2 +-
 2 files changed, 31 insertions(+), 24 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.

⏱️ Updated at 2023-12-15 15:30:27 +0000

tobyhodges

I've suggested modifying the title of the spoiler box, but in general this is excellent. Thanks @JeremyPike 🙌

episodes/08-connected-components.md

Co-authored-by: Toby Hodges <[email protected]>

JeremyPike · 2023-12-15T15:29:44Z

@tobyhodges yes that's a better header, change made :)

Auto-generated via {sandpaper} Source : 42bb13a Branch : main Author : Toby Hodges <[email protected]> Time : 2023-12-15 16:11:10 +0000 Message : Merge pull request #316 from bear-rsg/connected-components-suggestions Connected components suggestions

Auto-generated via {sandpaper} Source : c3b0ad5 Branch : md-outputs Author : GitHub Actions <[email protected]> Time : 2023-12-15 16:12:08 +0000 Message : markdown source builds Auto-generated via {sandpaper} Source : 42bb13a Branch : main Author : Toby Hodges <[email protected]> Time : 2023-12-15 16:11:10 +0000 Message : Merge pull request #316 from bear-rsg/connected-components-suggestions Connected components suggestions

mkcor · 2023-12-16T21:42:05Z

episodes/08-connected-components.md

@@ -316,24 +316,22 @@ labeled_image, count = connected_components(filename="data/shapes-01.jpg", sigma

 fig, ax = plt.subplots()
 plt.imshow(labeled_image)
-plt.axis("off");
+plt.axis("off")


I would keep the semicolon, so the output is not cluttered with text and memory addresses... But maybe the semicolon should be explained to learners? cf. mkcor/python-prog#8 (comment) 😉

Sure, sorry, I probably shouldn't have removed this! I don't use Jupyter much apart from for teaching so from my perspective I thought it might be confusing for some participants and I didn't mind seeing the limits of the axis printed. Happy to put back in with a brief explanation :)

mkcor · 2023-12-16T21:42:37Z

episodes/08-connected-components.md


-Here you might get a warning
+If you are using an old version of Matplotlib you might get a warning


Suggested change

If you are using an old version of Matplotlib you might get a warning

If you are using an older version of Matplotlib you might get a warning

mkcor · 2023-12-16T21:45:21Z

episodes/08-connected-components.md

+fig, ax = plt.subplots()
+plt.imshow(labeled_image, vmin=np.min(labeled_image), vmax=np.max(labeled_image))


Suggested change

fig, ax = plt.subplots()

plt.imshow(labeled_image, vmin=np.min(labeled_image), vmax=np.max(labeled_image))

plt.imshow(labeled_image, vmin=np.min(labeled_image), vmax=np.max(labeled_image))

Since fig and ax are not being used?

fig, ax = plt.subplots() is used here as I wanted to create a new figure rather than use the previous one, its used throughout the course even when fig and ax are not used so I'm hesitant to change to an alternative or remove the line. Maybe I don't need to create a new figure though, in which case the line can be removed as suggested?

Oh, ok, thanks for explaining! So, I would suggest you keep this line but replace the next one with:

ax.imshow(labeled_image, vmin=np.min(labeled_image), vmax=np.max(labeled_image))

I agree this is better but its not consistent with how its done in the rest of the course so perhaps this should be a different PR where all similar examples are changed at once.

I agree; I believe that we should use this "object-oriented (OO) style" throughout the lesson, since we create figure and axis objects now. At the moment, we are mixing up the OO-style with the pyplot-style [1]. For instance, 328d544 adds figure and axis objects, but we still have all the pyplot-style legacy. I would change, e.g.,

fig, ax = plt.subplots() plt.plot(bin_edges[0:-1], histogram) plt.title("Grayscale Histogram") plt.xlabel("grayscale value") plt.ylabel("pixels") plt.xlim(0, 1.0)

into

fig, ax = plt.subplots() ax.plot(bin_edges[0:-1], histogram) ax.set_title("Grayscale Histogram") ax.set_xlabel("grayscale value") ax.set_ylabel("pixels") ax.set_xlim(0, 1.0);

/cc @datacarpentry/image-processing-curriculum-maintainers

[1] https://matplotlib.org/stable/users/explain/quick_start.html#the-explicit-and-the-implicit-interfaces

Good catch, @mkcor. Please could you open a new issue where we can track this?

mkcor · 2023-12-16T21:45:44Z

episodes/08-connected-components.md

@@ -380,7 +385,7 @@ colored_label_image = ski.color.label2rgb(labeled_image, bg_label=0)

 fig, ax = plt.subplots()
 plt.imshow(colored_label_image)
-plt.axis("off");
+plt.axis("off")


Same comment as above.

mkcor · 2023-12-16T21:46:33Z

episodes/08-connected-components.md

@@ -402,7 +407,7 @@ How does changing the `sigma` and `threshold` values influence the result?
 ## Solution

 As you might have guessed, the return value `count` already
-contains the number of found images. So it can simply be printed
+contains the number of found objects in the image. So it can simply be printed


Suggested change

contains the number of found objects in the image. So it can simply be printed

contains the number of objects found in the image. So it can simply be printed

JeremyPike · 2023-12-18T09:08:27Z

@mkcor thanks for the nice review and great suggestions (see my comments above). However I think @tobyhodges has already merged this and I cant see an easy way to reopen. I'm happy to create a new PR with the changes if thats the best way?

mkcor · 2023-12-18T10:23:00Z

I'm happy to create a new PR with the changes if thats the best way?

You're most welcome, @JeremyPike! Yes, I think that creating a new PR would be the way to go. Thank you!

JeremyPike · 2023-12-18T13:05:01Z

I'm happy to create a new PR with the changes if thats the best way?

You're most welcome, @JeremyPike! Yes, I think that creating a new PR would be the way to go. Thank you!

Done, see #318

Auto-generated via {sandpaper} Source : c3b0ad5 Branch : md-outputs Author : GitHub Actions <[email protected]> Time : 2023-12-15 16:12:08 +0000 Message : markdown source builds Auto-generated via {sandpaper} Source : 42bb13a Branch : main Author : Toby Hodges <[email protected]> Time : 2023-12-15 16:11:10 +0000 Message : Merge pull request #316 from bear-rsg/connected-components-suggestions Connected components suggestions

JeremyPike added 4 commits December 12, 2023 15:07

display range text

b32977f

typo

b2a5044

naming function inputs

bbcc281

refactoring solution

50f56db

github-actions bot pushed a commit that referenced this pull request Dec 13, 2023

differences for PR #316

2918dec

rewording

57fbf84

JeremyPike marked this pull request as ready for review December 13, 2023 07:02

github-actions bot pushed a commit that referenced this pull request Dec 13, 2023

differences for PR #316

7f12b3c

tobyhodges approved these changes Dec 15, 2023

View reviewed changes

episodes/08-connected-components.md Outdated Show resolved Hide resolved

Updated spoiler heading

f072c71

Co-authored-by: Toby Hodges <[email protected]>

github-actions bot pushed a commit that referenced this pull request Dec 15, 2023

differences for PR #316

02f21d0

tobyhodges merged commit 42bb13a into datacarpentry:main Dec 15, 2023
3 checks passed

mkcor reviewed Dec 16, 2023

View reviewed changes

JeremyPike mentioned this pull request Dec 18, 2023

Review changes #318

Merged

mkcor mentioned this pull request Dec 19, 2023

Matplotlib is currently used in a hybrid way (explicit vs. implicit interfaces) #319

Closed

JeremyPike deleted the connected-components-suggestions branch February 2, 2024 10:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Connected components suggestions #316

Connected components suggestions #316

JeremyPike commented Dec 13, 2023 •

edited

Loading

github-actions bot commented Dec 13, 2023 •

edited

Loading

tobyhodges left a comment

JeremyPike commented Dec 15, 2023

mkcor Dec 16, 2023

JeremyPike Dec 18, 2023 •

edited

Loading

mkcor Dec 16, 2023

mkcor Dec 16, 2023

JeremyPike Dec 18, 2023 •

edited

Loading

mkcor Dec 18, 2023

JeremyPike Dec 18, 2023

mkcor Dec 19, 2023

tobyhodges Dec 19, 2023

mkcor Dec 16, 2023

mkcor Dec 16, 2023

JeremyPike commented Dec 18, 2023 •

edited

Loading

mkcor commented Dec 18, 2023

JeremyPike commented Dec 18, 2023


		Here you might get a warning
		If you are using an old version of Matplotlib you might get a warning

	If you are using an old version of Matplotlib you might get a warning
	If you are using an older version of Matplotlib you might get a warning

		fig, ax = plt.subplots()
		plt.imshow(labeled_image, vmin=np.min(labeled_image), vmax=np.max(labeled_image))

	contains the number of found objects in the image. So it can simply be printed
	contains the number of objects found in the image. So it can simply be printed

Connected components suggestions #316

Connected components suggestions #316

Conversation

JeremyPike commented Dec 13, 2023 • edited Loading

github-actions bot commented Dec 13, 2023 • edited Loading

Thank you!

Rendered Changes

tobyhodges left a comment

Choose a reason for hiding this comment

JeremyPike commented Dec 15, 2023

Choose a reason for hiding this comment

JeremyPike Dec 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JeremyPike Dec 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JeremyPike commented Dec 18, 2023 • edited Loading

mkcor commented Dec 18, 2023

JeremyPike commented Dec 18, 2023

JeremyPike commented Dec 13, 2023 •

edited

Loading

github-actions bot commented Dec 13, 2023 •

edited

Loading

JeremyPike Dec 18, 2023 •

edited

Loading

JeremyPike Dec 18, 2023 •

edited

Loading

JeremyPike commented Dec 18, 2023 •

edited

Loading