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

Adding Morphological Dilation and Erosion Algorithms #430

Closed
wants to merge 8 commits into from

Conversation

ayushbansal07
Copy link

@ayushbansal07 ayushbansal07 commented Feb 2, 2020

Description

Morphological Dilation and Erosion algorithms implemented (for grayscale images).
This PR is also meant to be my solution for the competency test for GSoC 2020.

Dilation/Erosion process is performed by laying the structuring element H on the image I and sliding it across the image in a manner similar to convolution.

  1. Morphological Dilation - The grayscale morphological dilation formula is written as follows :
    [𝐼⊕𝐻](𝑢,𝑣)=max(𝑖,𝑗)∈𝐻{𝐼(𝑢−𝑖,𝑣−𝑗)+𝐻(𝑖,𝑗)}
  2. Morphological Erosion - The grayscale morphological erosion formula is written as follows :
    [𝐼⊕𝐻](𝑢,𝑣)=min(𝑖,𝑗)∈𝐻{𝐼(𝑢−𝑖,𝑣−𝑗)+𝐻(𝑖,𝑗)}

References

Further development during GSoC 2020

Tasklist

  • Add basic test case(s)
  • Add more advanced test case(s)
  • Ensure all CI builds pass
  • Review and approve

Copy link
Member

@mloskot mloskot left a comment

Choose a reason for hiding this comment

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

Thanks for the interesting PR.

Here is my first quick review of it:

  1. I left number of comments to clear up trivial aspects of the proposed code first, so not much of commenting on the algorithms itself.

  2. Second, new features (or tweaks in behaviour of existing ones) must be covered with tests. Please, include test for the new functions you are proposing here.

  3. Is it necessary to include the two PNG files?

include/boost/gil/image_processing/morph.hpp Outdated Show resolved Hide resolved
include/boost/gil/image_processing/morph.hpp Outdated Show resolved Hide resolved
include/boost/gil/image_processing/morph.hpp Outdated Show resolved Hide resolved
include/boost/gil/image_processing/morph.hpp Outdated Show resolved Hide resolved
include/boost/gil/image_processing/morph.hpp Outdated Show resolved Hide resolved
include/boost/gil/image_processing/morph.hpp Outdated Show resolved Hide resolved
include/boost/gil/image_processing/morph.hpp Outdated Show resolved Hide resolved
include/boost/gil/image_processing/morph.hpp Outdated Show resolved Hide resolved
include/boost/gil/image_processing/morph.hpp Outdated Show resolved Hide resolved
include/boost/gil/image_processing/morph.hpp Outdated Show resolved Hide resolved
@mloskot mloskot added cat/feature New feature or functionality status/work-in-progress Do NOT merge yet until this label has been removed! labels Feb 2, 2020
…ts, removed two image files added previously. TODO: Add more tests
@ayushbansal07
Copy link
Author

ayushbansal07 commented Feb 3, 2020

Updates -

  1. Resolved all the trivial coding issues.
  2. Added a test for erosion and Dilation. (Will add more tests in the meanwhile, marked as TODO).
  3. Removed .png files.
    Also, I modified morphology.hpp to better handle the corner cases.

TODOs -

  • Add more tests.

@mloskot
Copy link
Member

mloskot commented Feb 3, 2020

@ayushbansal07

Updates -
...

This is looking good. Thanks for your efforts to improve this. I'll get another look and review near Wednesday.

TODOs -
...

I've taken the liberty and added those as the PR tasklist which you can also edit.


Another thing, you mentioned on the Boost mailing list you will be proposing those algorithms as part of your competency test. If you are working on this PR as the competency test, please update the description to make it clear this is your solution for the GSoC 2020 competency test.

Since working on PR may involve much more effort than working just on C++ code of the competency test solution, you are not expected to finish-polish the PR in order to count it for a completed competency test solution. In other words, your PR does not have to be approved and merged to consider the test completed. For the competency test, the gist of C++ code is most important.

Although you can postpone addressing further reviews and completion of this PR for later (e.g. during GSoC), you are very welcome to continue working on to make it ready for merging. It's your call.

/cc @stefanseefeld unless he does not agree and would like to clear my explanation.

@ayushbansal07 ayushbansal07 changed the title Adding Morphological Dilation and Erosion Algorithms GSoC 2020 competency test - Adding Morphological Dilation and Erosion Algorithms Feb 3, 2020
@ayushbansal07 ayushbansal07 changed the title GSoC 2020 competency test - Adding Morphological Dilation and Erosion Algorithms Adding Morphological Dilation and Erosion Algorithms Feb 3, 2020
@ayushbansal07
Copy link
Author

@mloskot

please update the description to make it clear this is your solution for the GSoC 2020 competency test.

I have updated the description.

I'd be happy to continue working on this and want to try my best to get it to merge before submitting my GSoC Proposal :)

Copy link
Member

@mloskot mloskot left a comment

Choose a reason for hiding this comment

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

I have added a bunch of nitpicks :-)

We have a prototype of benchmarks, currently in dedicated branch, that you may find interesteing. Here you can see comparison of various pixel iteration methods available in GIL


For me, this PR as it is now is a complete solution of your competency test and as I mentioned in #430 (comment) you are not required to make it a feature complete PR before the GSoC starts

If you still want to keep working on it, please consider add some some documentation e.g. Doxygen-like comments for erode and dilate routines.

example/morphology.cpp Outdated Show resolved Hide resolved
example/morphology.cpp Outdated Show resolved Hide resolved
include/boost/gil/image_processing/morphology.hpp Outdated Show resolved Hide resolved
include/boost/gil/image_processing/morphology.hpp Outdated Show resolved Hide resolved
include/boost/gil/image_processing/morphology.hpp Outdated Show resolved Hide resolved
include/boost/gil/image_processing/morphology.hpp Outdated Show resolved Hide resolved
include/boost/gil/image_processing/morphology.hpp Outdated Show resolved Hide resolved
include/boost/gil/image_processing/morphology.hpp Outdated Show resolved Hide resolved
@ayushbansal07
Copy link
Author

ayushbansal07 commented Feb 6, 2020

@mloskot
I have fixed the mentioned nitpicks.

Here you can see comparison of various pixel iteration methods available in GIL

Thank you for this interesting comparison. If you don't mind me asking, I wanted to know, just of out curiosity, what could be the reason behind such huge run time of 2d_y_iterator. Is it because each iteration would result in a cache miss?

@mloskot
Copy link
Member

mloskot commented Feb 6, 2020

@ayushbansal07

what could be the reason behind such huge run time of 2d_y_iterator.
Is it because each iteration would result in a cache miss?

Yes. I recommend to watch the original video lecture about GIL presented by Lubomir Bourdev
which is hosted on YouTube and linked from the documentation here https://boostorg.github.io/gil/html/tutorial/video.html
The Y-iterator and cache issue starts from around 8:00.

@ayushbansal07
Copy link
Author

@mloskot
I have added advanced test cases too. Please let me know further steps to complete this PR :-)

Copy link
Member

@mloskot mloskot left a comment

Choose a reason for hiding this comment

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

Could you also document template parameters using n D oxygen \tparam and description of type requirements or mention of concept, etc.

Optional

Unfortunately, we don't have structured prose documentation for the Image Processing features. However, perhaps you could also start scratching two Markdown files with prose for dilation and erosion, describing what's the algorithm does, how to use it, what are limitations, corner cases or not supported cases, include references or names of algorithms involved.

@ayushbansal07
Copy link
Author

Would it be okay if I did the documentation part later (during GSoC time)? I was asking this because I was planning to implement a separate class for Structuring Element as first step of my GSOC project and some of those corner cases, etc. would depend on the design of Structuring Element class. So, I was wondering if all of that could be done together afterwards.

@mloskot
Copy link
Member

mloskot commented Feb 9, 2020

Would it be okay if I did the documentation part later (during GSoC time)?
(...)
So, I was wondering if all of that could be done together afterwards.

Yes, certainly. That's what I also suggested in #430 (comment)

I think, your PR is completed from the competency test stand point, but to get it merged it will need that extra bit of work during the GSoC. So, let's keep it open as work in progress for now.

/cc @stefanseefeld

example/morphology.cpp Show resolved Hide resolved
include/boost/gil/image_processing/morphology.hpp Outdated Show resolved Hide resolved
include/boost/gil/image_processing/morphology.hpp Outdated Show resolved Hide resolved
include/boost/gil/image_processing/morphology.hpp Outdated Show resolved Hide resolved
include/boost/gil/image_processing/morphology.hpp Outdated Show resolved Hide resolved
include/boost/gil/image_processing/morphology.hpp Outdated Show resolved Hide resolved
include/boost/gil/image_processing/morphology.hpp Outdated Show resolved Hide resolved
test/core/image_processing/morphology.cpp Show resolved Hide resolved
test/core/image_processing/morphology.cpp Show resolved Hide resolved
@ayushbansal07
Copy link
Author

@mloskot Really sorry for the late response. I was a little busy with my semester exams. I have made the required changes :)

@mloskot
Copy link
Member

mloskot commented Feb 21, 2020

@ayushbansal07

Really sorry for the late response.

There is no need to be sorry. It's your PR, so you work on it at your own pace.

I have made the required changes :)

Please, hit the Resolve conversation button for each item you address or add comment explaining a thing is not addressed. This helps to keep completed review iterations cleared.

Warnings

@ayushbansal07 Please, could you also check compilation output of your tests and examples and clean up any compilation warnings caused by your code? Compilation of GIL code still outputs lots of warnings and we are (slowly) clearing that, but we definitely want to avoid introduction of new warnings.

@ayushbansal07
Copy link
Author

@mloskot I have made relevant changes to make sure that the Cl builds and has minimal warnings.

Also, FYI, I have checked Add more advanced test case(s) and Ensure all CI builds pass in the task list.

@mloskot
Copy link
Member

mloskot commented Feb 22, 2020

@ayushbansal07 in #430 (comment)

I'd be happy to continue working on this and want to try my best to get it to merge before submitting my GSoC Proposal

Thank you for the very good job you've done. I consider it as more than complete for your competency test.

Although the PR is very close to be approved and merged, we are not in rush
and I'd prefer to defer merging. I also mentioned it in #430 (review)

My reasoning is to :

  • give myself a bit more time to try your code out in action (I have only read-only reviewed it).
  • give other active/interested developers and users a chance to have a look and comment (/cc @stefanseefeld, @lpranam, @simmplecoder, @miralshah365, @chhenning).
  • leave it for you, in case you are approved as student, as an easy first task to completed as part of your GSoC work (an early success is an important element of GSoC :-))

Regardless if you will participate during GSoC or not, since this PR is pretty much completed, I'm fairly sure your contribution will be merged eventually.

@mloskot mloskot added core boost/gil google-summer-of-code All items related to GSoC activities labels Feb 22, 2020
@ayushbansal07
Copy link
Author

ayushbansal07 commented Feb 26, 2020

@mloskot
Thank you for your reviews. I too agree to defer the merging mostly because of the last reason that you mentioned :)

Also, if it is not much of a trouble for you, please feel free to mention me in any discussion that might potentially help me understand more about GIL.

Thank you again! :)

@mloskot
Copy link
Member

mloskot commented Feb 26, 2020

@ayushbansal07

please feel free to mention me in any discussion that might potentially help me understand more about GIL

Sure, but nothing immediately comes to mind. Feel free to browse the issues, check the good first issue list.

For understanding GIL, I highly recommend getting familiar with these topics from GIL docs:

  • The video lecture
  • The three tutorials
  • These from the design guide:
    • Channel
    • Color Space and Layout
    • Color Base
    • Pixel
    • Pixel Iterator
    • Pixel Locator
    • Image View
    • Image

These are essential parts to understand to be comfortable with using GIL. Mind you, I'm not suggesting to memorise the API or any details of implementation, but learn vocabulary of GIL, what are the building blocks, features they offer, how to use them, when to use them.

If anything needs clarifying, don't hesitate to ask questions on the boost-gil list.
If you think anything is missing or wrong, please, open new issue.

This should make a busy GSoC preparation for next days :)

@mloskot
Copy link
Member

mloskot commented Jan 25, 2021

This is being replaced with more feature-complete morphological operations implementation in #541

Thanks @ayushbansal07 for your work here.

@mloskot mloskot closed this Jan 25, 2021
@mloskot mloskot removed the status/work-in-progress Do NOT merge yet until this label has been removed! label Jan 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cat/feature New feature or functionality core boost/gil google-summer-of-code All items related to GSoC activities
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants