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

WIP NEW histogram backprojection #3590

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

sciunto
Copy link
Member

@sciunto sciunto commented Dec 15, 2018

Description

This PR replaces and enhances #979

TODO list

  • Move code to correct location
  • improve example
  • add unitests
  • Fix API & docstring
  • suppress specific code for uint8, use our histogram function

Checklist

[It's fine to submit PRs which are a work in progress! But before they are merged, all PRs should provide:]

[For detailed information on these and other aspects see scikit-image contribution guidelines]

References

[If this is a bug-fix or enhancement, it closes issue # ]
[If this is a new feature, it implements the following paper: ]

For reviewers

(Don't remove the checklist below.)

  • 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.
  • Consider backporting the PR with @meeseeksdev backport to v0.14.x

@pep8speaks
Copy link

pep8speaks commented Dec 15, 2018

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

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

Line 112:80: E501 line too long (81 > 79 characters)
Line 115:80: E501 line too long (81 > 79 characters)

Comment last updated at 2020-01-15 06:18:06 UTC

@sciunto sciunto added this to the 1.0 milestone Dec 20, 2018
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.

@sciunto looking good! A few PEP8 issues (e.g. camelCase). Let us know when you think the WIP can come down and we can do a full review.

@sciunto
Copy link
Member Author

sciunto commented Sep 17, 2019

This is not ready yet imho. I just fixed something that crossed a grep on my repository. :)

@emmanuelle
Copy link
Member

@sciunto what else did you want to do for this PR? I noticed that some variable names (B, R...) did not correspond to our conventions of using meaningful names. But apart from that it looks good. Happy to take over with your guidance, if you prefer.

@sciunto
Copy link
Member Author

sciunto commented Jan 15, 2020

@emmanuelle I do not like the way the color detection is done. I believe that we consider this as "magic" (ie guess from dimensions). Instead, kwarg is the function's signature would be better.

Also, I do not like that images are forced to be with a 8 bits precision. This must be generalized as well.

I agree also for some cleanup on the variable names. Unitests are missing as well.

Still a significant effort to do, if you would like to help, I'll be happy of it :)

skimage/segmentation/backproject.py Outdated Show resolved Hide resolved
skimage/segmentation/backproject.py Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants