10000 Fix corrupt floodfill by FirefoxMetzger · Pull Request #4948 · scikit-image/scikit-image · GitHub
[go: up one dir, main page]
Skip to content

Fix corrupt floodfill #4948

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 8 commits into from
Sep 7, 2020
Merged

Fix corrupt floodfill #4948

merged 8 commits into from
Sep 7, 2020

Conversation

FirefoxMetzger
Copy link
Contributor

Description

Closes: #4918

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 Aug 29, 2020 •
edited
Loading

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

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-09-07 12:16:10 UTC

@rfezzani rfezzani mentioned this pull request Aug 31, 2020
Closed
@sciunto sciunto added this to the 0.18 milestone Sep 6, 2020
@sciunto sciunto added action: mrg+1 🩹 type: Bug fix Fixes unexpected or incorrect behavior labels Sep 6, 2020
jni
jni reviewed Sep 6, 2020
View reviewed changes
@FirefoxMetzger FirefoxMetzger requested a review from jni September 6, 2020 15:41
jni
jni reviewed Sep 6, 2020
View reviewed changes
jni
jni reviewed Sep 6, 2020
View reviewed changes
skimage/morphology/tests/test_flood_fill.py Outdated
@@ -219,7 +219,7 @@ def test_selem():
[0, 255, 255, 255, 255, 255],
[0, 255, 255, 255, 255, 255],
[0, 255, 255, 255, 255, 255],
[0, 0, 0, 0, 0, 0]], dtype=np.uint8)
[0, 0, 0, 0, 0, 0]], dtype=np.uint8)
Copy link
Member

Choose a reason for hiding this comment

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

Could you please revert this change?

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
[0, 0, 0, 0, 0, 0]], dtype=np.uint8)
[0, 0, 0, 0, 0, 0]], dtype=np.uint8)

jni
jni reviewed Sep 6, 2020
View reviewed changes
skimage/morphology/tests/test_flood_fill.py Outdated
[255, 255, 255, 255, 255, 0],
[255, 255, 255, 255, 255, 0],
[255, 255, 255, 255, 255, 0],
[255, 255, 255, 255, 255, 0]], dtype=np.uint8)
Copy link
Member

Choose a reason for hiding this comment

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

And here. (Sorry, this suggestion is harder to do on the web interface 😬)

jni
jni reviewed Sep 6, 2020
View reviewed changes
jni
jni approved these changes Sep 6, 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.

@FirefoxMetzger other than some very minor formatting/naming suggestions, this is ready for me!

The Travis failures appear unrelated. One is a sporadic failure in the rank filters tests, the other is the mac build hanging which we have encountered elsewhere.

@emmanuelle
Copy link
Member

Also approving once's @jni's small comments have been addressed.

@FirefoxMetzger
Copy link
Contributor Author

@jni @emmanuelle done. Let's wait for the tests to pass and then this should be good.

@FirefoxMetzger
Copy link
Contributor Author

@jni LGTM

@emmanuelle
Copy link
Member

Thank you @FirefoxMetzger , merging!

@emmanuelle emmanuelle merged commit 1092493 into scikit-image:master Sep 7, 2020
@jni
Copy link
Member
jni commented Sep 12, 2020

@meeseeksdev backport to v0.17.x

@lumberbot-app
Copy link
lumberbot-app bot commented Sep 12, 2020

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

  1. Checkout backport branch and update it.
$ git checkout v0.17.x
$ git pull
  1. Cherry pick the first parent branch of the this PR on top of the older branch:
$ git cherry-pick -m1 10924934d65b7d0ec3a52a6324276b56fabb079a
  1. You will likely have some merge/cherry-pick conflict here, fix them and commit:
$ git commit -am 'Backport PR #4948: Fix corrupt floodfill'
  1. Push to a named branch :
git push YOURFORK v0.17.x:auto-backport-of-pr-4948-on-v0.17.x
  1. Create a PR against branch v0.17.x, I would have named this PR:

"Backport PR #4948 on branch v0.17.x"

And apply the correct labels and milestones.

Congratulation you did some good work ! Hopefully your backport PR will be tested by the continuous integration and merged soon!

If these instruction are inaccurate, feel free to suggest an improvement.

@lumberbot-app lumberbot-app bot added the Still Needs Manual Backport MrMeeseeks-managed label label Sep 12, 2020
@jni
Copy link
Member
jni commented Sep 12, 2020

Dang. LOL

@jni jni mentioned this pull request Sep 12, 2020
Merged
@FirefoxMetzger FirefoxMetzger deleted the fix-corrupt-floodfill branch October 30, 2020 15:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sciunto sciunto sciunto approved these changes

@jni jni jni approved these changes

Assignees
No one assigned
Labels
🩹 type: Bug fix Fixes unexpected or incorrect behavior Still Needs Manual Backport MrMeeseeks-managed label
Projects
None yet
Milestone
0.18
Development

Successfully merging this pull request may close these issues.

Memory corruption in flood_fill
5 participants
@FirefoxMetzger @pep8speaks @emmanuelle @jni @sciunto
0