8000 [MRG] Update documentation to new data.camera() by kalpanab-psg · Pull Request #5018 · scikit-image/scikit-image · GitHub
[go: up one dir, main page]
Skip to content

[MRG] Update documentation to new data.camera() #5018

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Oct 14, 2020
Merged

[MRG] Update documentation to new data.camera() #5018

merged 7 commits into from
Oct 14, 2020

Conversation

kalpanab-psg
Copy link
Contributor

Description

Fixes: #4993

Checklist

For reviewers

  • Check that the PR title is short, concise, and will make sense 1 year
    later.
  • Check that new functions are imported in corresponding __init__.py.
  • Check that new features, API changes, and deprecations are mentioned in
    doc/release/release_dev.rst.

@pep8speaks
Copy link
pep8speaks commented Oct 12, 2020 •
edited
Loading

Hello @kalpanab-psg! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 74:80: E501 line too long (80 > 79 characters)

Comment last updated at 2020-10-13 16:39:42 UTC

@kalpanab-psg kalpanab-psg changed the title [MRG] Update documentation to new data.camera() [WIP] Update documentation to new data.camera() Oct 12, 2020
jni
jni reviewed Oct 13, 2020
View reviewed changes
doc/examples/segmentation/plot_floodfill.py Outdated
@@ -54,25 +54,26 @@
cameraman = data.camera()

# Change the cameraman's coat from dark to light (255). The seed point is
# chosen as (200, 100),
light_coat = flood_fill(cameraman, (200, 100), 255, tolerance=10)
# chosen as (150,170, 100),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# chosen as (150,170, 100),
# chosen as (170, 150),

jni
jni reviewed Oct 13, 2020
View reviewed changes
doc/examples/segmentation/plot_multiotsu.py
ax[2].imshow(regions, cmap='Accent')
ax[2].imshow(regions, cmap='jet')
Copy link
Member

Choose a reason for hiding this comment

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

I think this change is probably not necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think someone mentioned a color change to lessen the pink effect here ->#4993 (comment).
The cmap with jet parameter looks better than Accent.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, fair enough. I just have a severe allergy to jet. LOL I think even if not accent we should use one of the qualitative colormaps found in this page

@jni
Copy link
Member
jni commented Oct 13, 2020

@kalpanab-psg Thanks for these! I'm not sure why the circleci builds failed. I also don't understand why I need to log in to view the Circle-CI build log — @scikit-image/core any ideas?

But, since we can't see the output online, would you mind posting screenshots of the figure outputs of your changes here, to make review easier?

@kalpanab-psg
Copy link
Contributor Author

Multi otsu
multi_otsu

Floodfill - seed point changed to (150,155)
floodfill

watershed-- image changed to eagle
watershed

@alexdesiqueira alexdesiqueira mentioned this pull request Oct 13, 2020
Closed
jni
jni reviewed Oct 13, 2020
View reviewed changes
doc/examples/segmentation/plot_floodfill.py
fig, ax = plt.subplots(ncols=2, figsize=(10, 5))

ax[0].imshow(cameraman, cmap=plt.cm.gray)
ax[0].set_title('Original')
ax[0].axis('off')

ax[1].imshow(light_coat, cmap=plt.cm.gray)
ax[1].plot(150, 170, 'ro') # seed point
ax[1].plot(150, 155, 'ro') # seed point
Copy link
Member

Choose a reason for hiding this comment

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

This should be (155, 150) because plotting axis order is reversed compared to NumPy axis order in matplotlib.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But when we see the output image the (x,y) values are showed as (150,155) in red dot.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will check and revert regarding other cmap option. Thanks for the suggestions

jni
jni approved these changes Oct 13, 2020
View reviewed changes
Copy link
Member
@jni jni left a comment

Choose a reason for hiding this comment

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

Thanks so much for the screenshots! I've suggested a minor code fix re seed point, and an optional suggestion about the multiotsu colormap — not something I feel that strongly about.

@jni
Copy link
Member
jni commented Oct 14, 2020

@kalpanab-psg thanks for the fix! I'm going to merge this now! If you find a better colormap, please open a new PR. =)

@jni jni merged commit ef5ac57 into scikit-image:master Oct 14, 2020
@kalpanab-psg kalpanab-psg changed the title [WIP] Update documentation to new data.camera() [MRG] Update documentation to new data.camera() Oct 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jni jni jni approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Update documentation to new data.camera()
3 participants
@kalpanab-psg @pep8speaks @jni
0