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

edgeSnap method #391

Closed
wants to merge 14 commits into
base: develop
from

Conversation

Projects
None yet
3 participants
@vighneshbirodkar
Contributor

vighneshbirodkar commented Apr 12, 2013

To implement feature request in #237

Added edgeSnap method in Image Class

My first pull request ever. Let me know if I have missed anything. I'll make the changed ASAP

@jayrambhia

This comment has been minimized.

Show comment
Hide comment
@jayrambhia

jayrambhia Apr 14, 2013

Contributor

Why have you added so many of new lines? You need to work on the indentation. SimpleCV uses 4 spaces for indentation. You have used 4 spaces somewhere and 2 spaces somewhere. SimpleCV uses CamelsCase. You should read PEP8. There's already a line class in Features so there's no need to define a new one. You should check it out. ImageClass just has Image and ImageSet so you can't add some new class into it. It won't get merged. There are some rules and guidelines that you need to follow while contributing to some repository. You really seem new to Python. Instead of that while loop you could have easily added a for loop (much better option). Even your docs lack indentation. Returning None is always safer than returning an empty list. Basically, you need to work on your Python skills and learn guidelines.

Contributor

jayrambhia commented Apr 14, 2013

Why have you added so many of new lines? You need to work on the indentation. SimpleCV uses 4 spaces for indentation. You have used 4 spaces somewhere and 2 spaces somewhere. SimpleCV uses CamelsCase. You should read PEP8. There's already a line class in Features so there's no need to define a new one. You should check it out. ImageClass just has Image and ImageSet so you can't add some new class into it. It won't get merged. There are some rules and guidelines that you need to follow while contributing to some repository. You really seem new to Python. Instead of that while loop you could have easily added a for loop (much better option). Even your docs lack indentation. Returning None is always safer than returning an empty list. Basically, you need to work on your Python skills and learn guidelines.

@vighneshbirodkar

This comment has been minimized.

Show comment
Hide comment
@vighneshbirodkar

vighneshbirodkar Apr 14, 2013

Contributor

Thanks for the critique.
I wasn't aware of PEP8, will definitely follow it from now on.

The main intention behind adding the class was not to do computations for further points if they are not required , hence the iterator. I'll use the already defined bresenham code then. I looked at the Line class, I fail to see how it would server my purpose though.

I've been using Python for a while, I'm very new to contributing though. I agree that while loop is a mistake

And I'll send another pull request soon.

Contributor

vighneshbirodkar commented Apr 14, 2013

Thanks for the critique.
I wasn't aware of PEP8, will definitely follow it from now on.

The main intention behind adding the class was not to do computations for further points if they are not required , hence the iterator. I'll use the already defined bresenham code then. I looked at the Line class, I fail to see how it would server my purpose though.

I've been using Python for a while, I'm very new to contributing though. I agree that while loop is a mistake

And I'll send another pull request soon.

@jayrambhia

This comment has been minimized.

Show comment
Hide comment
@jayrambhia

jayrambhia Apr 14, 2013

Contributor

Bresenham line should do it. Plus, you could make changes in the Line class.. Don't need to send another pull.. just change in the same branch and push it. GitHub will take care of it and update this pull request

Contributor

jayrambhia commented Apr 14, 2013

Bresenham line should do it. Plus, you could make changes in the Line class.. Don't need to send another pull.. just change in the same branch and push it. GitHub will take care of it and update this pull request

@vighneshbirodkar

This comment has been minimized.

Show comment
Hide comment
@vighneshbirodkar

vighneshbirodkar Apr 14, 2013

Contributor

Okay ! Any flaws with the logic according to you ?

Contributor

vighneshbirodkar commented Apr 14, 2013

Okay ! Any flaws with the logic according to you ?

@vighneshbirodkar

This comment has been minimized.

Show comment
Hide comment
@vighneshbirodkar

vighneshbirodkar Apr 15, 2013

Contributor

@jayrambhia
I have made the changes. Tell me if there is still something lacking. I made improvements to the procedure.
The results are much more reliable now.

sc3
sc2

Contributor

vighneshbirodkar commented Apr 15, 2013

@jayrambhia
I have made the changes. Tell me if there is still something lacking. I made improvements to the procedure.
The results are much more reliable now.

sc3
sc2

@jayrambhia

This comment has been minimized.

Show comment
Hide comment
@jayrambhia

jayrambhia Apr 15, 2013

Contributor

This is really great. Your method returns a list. Instead of that it should return a FeatureSet object. FeatureSet is derived from list so it severs the purpose. and it'd be easier for the user to draw the edge snap line.

fs = img.edgesnap()
fs.draw()
Contributor

jayrambhia commented Apr 15, 2013

This is really great. Your method returns a list. Instead of that it should return a FeatureSet object. FeatureSet is derived from list so it severs the purpose. and it'd be easier for the user to draw the edge snap line.

fs = img.edgesnap()
fs.draw()
@vighneshbirodkar

This comment has been minimized.

Show comment
Hide comment
@vighneshbirodkar

vighneshbirodkar Apr 15, 2013

Contributor

on it

Contributor

vighneshbirodkar commented Apr 15, 2013

on it

@vighneshbirodkar

This comment has been minimized.

Show comment
Hide comment
@vighneshbirodkar

vighneshbirodkar Apr 15, 2013

Contributor

Done !!

Contributor

vighneshbirodkar commented Apr 15, 2013

Done !!

@kscottz

This comment has been minimized.

Show comment
Hide comment
@kscottz

kscottz Apr 15, 2013

Contributor

So I just played with this and I'm really impressed. I need one thing before I merge it in. I need this to run with arbitrary binary images and not just canny edges. This is to say that I don't want to run a canny edge detect over the image, I just want the method to assume the image is binary and go from there. We may want to put a check in there so that we don't apply this to a color image. You can totally use this method with a threshold operation or a watershed so I don't want to limit it to just edges.

I really like the use case example.

Contributor

kscottz commented Apr 15, 2013

So I just played with this and I'm really impressed. I need one thing before I merge it in. I need this to run with arbitrary binary images and not just canny edges. This is to say that I don't want to run a canny edge detect over the image, I just want the method to assume the image is binary and go from there. We may want to put a check in there so that we don't apply this to a color image. You can totally use this method with a threshold operation or a watershed so I don't want to limit it to just edges.

I really like the use case example.

@vighneshbirodkar

This comment has been minimized.

Show comment
Hide comment
@vighneshbirodkar

vighneshbirodkar Apr 16, 2013

Contributor

@kscottz The travis build isn't showing up . Can you still merge this ? Or should I send another pull request ?

Contributor

vighneshbirodkar commented Apr 16, 2013

@kscottz The travis build isn't showing up . Can you still merge this ? Or should I send another pull request ?

@kscottz

This comment has been minimized.

Show comment
Hide comment
@kscottz

kscottz Apr 17, 2013

Contributor

Here is a little point that we should probably imgArray = self.getNumpy() could probably stand to be self.getGrayNumpy() just for a possible little speedup. Also you appear to have forgotten to back out the t1,t2 parameters to all your methods. We should get rid of those and everything should be set to merge.

Contributor

kscottz commented Apr 17, 2013

Here is a little point that we should probably imgArray = self.getNumpy() could probably stand to be self.getGrayNumpy() just for a possible little speedup. Also you appear to have forgotten to back out the t1,t2 parameters to all your methods. We should get rid of those and everything should be set to merge.

@kscottz

This comment has been minimized.

Show comment
Hide comment
@kscottz

kscottz Apr 19, 2013

Contributor

I am going to close this in favor of the new method that I will test shortly.

Contributor

kscottz commented Apr 19, 2013

I am going to close this in favor of the new method that I will test shortly.

@kscottz kscottz closed this Apr 19, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment