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

Add morphology operation #874

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

ArchdukeTim
Copy link

@ArchdukeTim ArchdukeTim commented Dec 28, 2017

This adds the opencv morphologyEx function. More info here

@codecov-io
Copy link

codecov-io commented Dec 28, 2017

Codecov Report

Merging #874 into master will increase coverage by 0.1%.
The diff coverage is 66.66%.

@@             Coverage Diff             @@
##             master     #874     +/-   ##
===========================================
+ Coverage     51.71%   51.81%   +0.1%     
- Complexity     1158     1160      +2     
===========================================
  Files           247      248      +1     
  Lines          7812     7862     +50     
  Branches        533      533             
===========================================
+ Hits           4040     4074     +34     
- Misses         3591     3607     +16     
  Partials        181      181

@JLLeitschuh
Copy link
Member

I'm good with this. I assume you tested this and it worked the way you wanted?

@ArchdukeTim
Copy link
Author

Yeah. Although I'm not sure how you feel about my hard coded enums. I couldn't figure out where to get that value from since the other ones use 'generated' code but I can't find it.

@ArchdukeTim
Copy link
Author

How do I add things to code generation? It's not very useful without it

@@ -0,0 +1,14 @@
@staticmethod
Copy link
Author

Choose a reason for hiding this comment

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

I tested this and it works

Copy link
Author

@ArchdukeTim ArchdukeTim left a comment

Choose a reason for hiding this comment

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

I didn't test the c++ or java generated code

final int kernelType = typeSocket.getValue().get().value;

outputSocket.setValue(opencv_imgproc.getStructuringElement(kernelType, new Size(widthValue,
heightValue)));
Copy link
Member

Choose a reason for hiding this comment

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

Won't this allocate a new matrix every time this perform method is called.
Don't you instead want to pass in an existing matrix so the memory can be reused?

Copy link
Author

@ArchdukeTim ArchdukeTim Dec 28, 2017

Choose a reason for hiding this comment

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

Ideally you'd create the kernel once with all the variables, but idk how to get that behavior.

Do you mean the Size matrix?

Copy link
Member

Choose a reason for hiding this comment

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

There should be an overload that takes a dest as a parameter.

Copy link
Author

@ArchdukeTim ArchdukeTim Dec 29, 2017

Choose a reason for hiding this comment

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

I'm not sure I understand. Is there an example in the code I can copy?

Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately no.
What we are trying to avoid is allocating new memory on every call.
So there should be an alternative method called getStructuringElement that takes an additional arg called dest. You can pass a pre-allocated matrix so we aren't allocating more memory on every call.

Copy link
Author

Choose a reason for hiding this comment

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

It's december of the next year, which means it's my turn to comment 😂

Going back and looking through this, I understand what we are going for, but not sure how that functionality is created in GRIP. The kernel creating isn't part of the pipeline, just the setup, although I think i'm combining the code generation with the actual application..lemme see what I can come up with for the application part and then I'll worry about code gen later

@ArchdukeTim
Copy link
Author

So i updated the python code to alleviate this I think. Instead of putting the function in process, it puts it in __init__. However the java code seems to allocate every variable in process which is...weird. I'm not entirely sure if this is a good idea or not, but it's more close to the code I would write freehand

@JLLeitschuh
Copy link
Member

@SamCarlberg Do you have any time to finish up this PR? I don't really at this exact moment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants