Added connected components #3
Conversation
There was a problem hiding this comment.
Great work Ryan, you caught a lot of stylistic things that I wasn't anticipating for new members, so I'm very happy with this! The review here is mainly a stylistic review, and once you're done making the changes, I will then review your code for correctness.
|
|
||
| namespace found { | ||
|
|
||
| struct Point { |
There was a problem hiding this comment.
Use Vec2 instead, which is found in attitude-utils.hpp
| struct Edge { | ||
| int magnitude; // number of points in this edge | ||
| std::vector<Point> points; | ||
| // int Xmin, Xmax, Ymin, Ymax; |
There was a problem hiding this comment.
Consider adding these metadata fields in. It may be useful in the future to distinguish between edges that might be part of Earth (in which case, one of these fields should be near the border), and others. Also, relocate the Edge struct to the style.hpp file.
|
|
||
| std::vector<Edge> connectedPoints; | ||
|
|
||
| for (const auto& pair : params) { |
There was a problem hiding this comment.
Do not use the auto keyword unless it is extremely inconvenient (i.e. for a variable that holds a function)
| @@ -0,0 +1,265 @@ | |||
| #include <stdio.h> | |||
There was a problem hiding this comment.
rename files to edge.cpp and edge.hpp for now
| // int Xmin, Xmax, Ymin, Ymax; | ||
| }; | ||
|
|
||
| std::vector<Edge> NaiveConnectedComponent(unsigned char *image, int imageWidth, int imageHeight) { // const?? |
There was a problem hiding this comment.
Unless you plan on making a better version, it might be better to name this just as ConnectedComponents or such
| // Initial labeling of centroids | ||
| for(long i = 1; i < imageHeight * imageWidth; i++) { | ||
| // This is specific to the centroiding algo, which tests if this is even part of a centroid | ||
| if(image[i] > threshold) { |
There was a problem hiding this comment.
Above, comment with: TODO: Edit this for compatibility with edge filtering
|
|
||
| // make an array with the same dimensions of the image, but it shows connected components using numbers | ||
| unsigned char *edges = (unsigned char *) std::malloc(imageWidth * imageHeight * sizeof(unsigned char)); | ||
| edges[0] = (image[0] > threshold) ? 1 : 0; |
There was a problem hiding this comment.
Above, comment with: TODO: Edit this for compatibility with edge filtering
| #include <string.h> | ||
| #include <cassert> | ||
| #include <cstdlib> | ||
|
|
There was a problem hiding this comment.
#include your .hpp file here. You'll then notice that there is a double definition, but the below comments will fix that.
| }; | ||
|
|
||
| // Function prototype | ||
| std::vector<Edge> NaiveConnectedComponent(unsigned char *image, int imageWidth, int imageHeight); |
There was a problem hiding this comment.
Please put a more descriptive function comment here. There should be:
- General Description of the Funciton
- Parameter Description
- Outputs
- Any modifications on the parameters
- Preconditions
- (optional) Postconditions
You can use @ tags for this. More info here: https://darkognu.eu/programming/tutorials/doxygen_tutorial_cpp/.
|
|
||
| } | ||
|
|
||
| #endif |
There was a problem hiding this comment.
I know you have completed tests on this function, so I will need you to formalize that in the test suite. That can be found in "found/test/test.cpp" (No .hpp file needed here). example.cpp will help you figure out how to write tests using C++'s CHECK framework (I'm open to other test suites if its better by the way).
No description provided.