-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Add rolling ball algorithm for background substraction #4851
Add rolling ball algorithm for background substraction #4851
Conversation
Hello @FirefoxMetzger! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2020-10-30 09:03:58 UTC |
Co-authored-by: Marianne Corvellec <marianne.corvellec@ens-lyon.org>
Co-authored-by: Marianne Corvellec <marianne.corvellec@ens-lyon.org>
…t-image into rolling_ball_filter
@FirefoxMetzger drawing your attention to the above review comment, you might have missed it in the midst of the rest ;) |
@mkcor Is there a tutorial/documentation on how to run the tests locally? That way, I can at least make sure they all pass before pushing commits |
Sure! In the contribution guidelines, you can find a section on testing: https://scikit-image.org/docs/stable/contribute.html#testing |
Note that the float input case is quite common, so we will need to figure out how to handle this. We can: only support in images (error otherwise), or convert float to it images, or figure out a float-specific version. |
We can start with the first option, which lets the possibility for improvements. |
I'm back from my vacation. @jni I've updated the function as you suggested. I moved the code to I noticed that 0.18 is still not released. Does this mean this PR will be on ice until release? |
@stefanv Is there any ETA or timeline on how we want to proceed with this feature? I still think it's quite the cool feature, and it would be a real shame to not get it added. |
@jni Can we get this in for the next release still? |
Yes, @stefanv if you can comment on the API choices we can get it in. We were holding off so we could have time to discuss the API at length. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you again @FirefoxMetzger , just left a couple of suggestions 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I realise that currently the API matches my preference (@emmanuelle and @mkcor in the meeting agree that it's fine to think of a general ball 😂) so I'm happy to merge as-is. =) Note that if #4951 merges before this, the test using cells.tif
will need to be updated, and vice versa.
@FirefoxMetzger as always, thank you so much for all your hard work and for your patience!!! 🙏 🙏 🙏 🙏
@jni generalized ball 😇 |
Thanks! Co-authored-by: Riadh Fezzani <rfezzani@gmail.com>
Thanks, @rfezzani ! you have a really good eye for details. Your suggestions and reviews are always appreciated. Thanks, @jni for the praise. I will check #4951 and see what needs to be changed here. When reading the code again, I noticed two improvements (refactoring for consistency) that can be made to the docstrings of |
path = data.image_fetcher.fetch('data/cells.tif') | ||
image = io.imread(path)[:5, ...] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops:
path = data.image_fetcher.fetch('data/cells.tif') | |
image = io.imread(path)[:5, ...] | |
image = data.cells3d()[:5, 1, :, :] |
I'll get this right eventually but I think you get the point, I'll go back in my hole now 😂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @FirefoxMetzger, once CI is green and you feel that it's fine, we are ready to go 😉
I finished the changes to the docstrings. I merged the changes from master (to enable |
@rfezzani Would you mind having a look at the Travis CI errors for Mac and telling me if you can make any sense of it? The issue seems to be that the script can't find a python3.7 executable (it should be python3 if I read the logs correctly), and then crashes. I didn't modify any CI code, so I'm doubtful if this PR is the cause. Is there any known issue with the CI for Mac? If it's not an issue that needs fixing in this PR, I'm happy with the current version. Otherwise, I could do with a pointer to tell me where to look for the issue. |
This PR is not the only affected 🙁. Travis CI is unfortunately causing a lot of trouble 😭.. |
Boom. Many thanks @FirefoxMetzger! 🎉
This is crazy! It's the same with me! It's not this PR, this is the case in master. Bizarrely,
I'll make an issue! |
🚀 💯 |
Beautifully done. Thank you @FirefoxMetzger and reviewers! |
This pull request has been mentioned on Image.sc Forum. There might be relevant details there: https://forum.image.sc/t/background-subtraction-in-scikit-image/39118/7 |
Description
Adds an example to implement the rolling ball algorithm.
Relevant Issue: #3538
Relevant Paper: Biomedical Image Processing
For reviewers
later.
__init__.py
.doc/release/release_dev.rst
.