Feature: Implementing Algorithm Superclass #47

MahirEmran · 2025-10-28T20:20:29Z

Description

Resolves #23

Added EdgeFilteringAlgorithm into DistancePipelineExecutor constructor
Edited DistancePipeline to have 4 items
Added edge_filters.hpp w/ ProvideEdgeFilteringAlgorithm and ModifyingPipeline
Added OPT_ASSIGN flags

Artifacts for PR #47 (DO NOT CHANGE)

nguy8tri · 2025-11-04T03:14:29Z

Hi, so you may notice that your coverage is failing for unapparent reasons. A recent bug that was introduced into Linux is the likely culprit, but unfortunately, we don't know what the bug is or how to fix it. However, we do know a way to go around it. I have noticed that you've added some new tests to pipeline-tests.cpp. You should try moving them to nominal-pipeline-test.cpp to see if it fixes the issue. If that doesn't work, try making all the pipeline tests in one file.

Note

For reference, there seems to be an issue where instantiating templates across multiple locations makes for uncombined or incomplete coverage data. Thus, the solution to this issue is to only test a particular instantiation in one file. Attempting to test them in both files (as you've done for DistancePipeline) will for some reason make gcovr believe that some things are not fully covered.

Also, I highly encourage you to work with mocks instead of instantiating new objects. This may seem odd at first, but it is a very robust way to mock behavior instead of making full on implementations, however small it looks. For instance, all your test stages within sequential-pipeline can easily be reformed into:

EXPECT_CALL(mock, function(args...)).WillOnce(testing::Return(your stuff))

As odd as it seems, it is an industry essential in nominal software development, but also just a good way to not worry about making test classes. This whole thing about mocking falls under something called Dependency Injection, used everywhere in the real world. To learn more about it, please look here: Mocking and Dependency Injection. You should also look at the purpose of the providers folder, detailed in Executor Injection, which is NEVER to be used directly in test cases (only indirectly through integration tests) :).

For reference the two functions that were not called (and why your coverage is failing in Github) are (there is almost no logic to this by the way):

[found::SequentialPipeline<std::pair<found::EulerAngles, found::EulerAngles>, found::Quaternion, 1ul>::Run(std::pair<found::EulerAngles, found::EulerAngles> const&) (line 222)]
[found::SequentialPipeline<std::vector<found::LocationRecord, std::allocatorfound::LocationRecord >, std::vector<found::LocationRecord, std::allocatorfound::LocationRecord >, 2ul>::Run(std::vector<found::LocationRecord, std::allocatorfound::LocationRecord > const&) (line 222)]

nguy8tri

I was going to leave this to Josh, but I realized some confusion and tough issues warrant my input on this.

I want to first commend you on doing all this, its not easy to pick up a new code space and to run with it. That being said, there are many things that must be corrected. I think you will see that a lot of this could've been avoided by looking over the thourough documentation in FOUND (but who does that am I right haha?).

A summary of what I saw:

I saw many opportunities for Dependency Mocking. Trust me, its worth your time, so take some time to learn how its used via the links in the comments.
I also saw a lack of understanding around Dependency Injection Framework that's used in the code. No worries, that's not even taught in the 4 years (in my case 5) that you're at UW.
Confusion around the coverage. That's understandable, we'll work through it together (for reference, we faced the same issue before, and it took me 3 months to figure it out)
Unnecessary code that is good in theory but useless/unoptimized in production. That will take a while to shake itself out (I'm still doing that myself)
Nitpicky style things to help you conform to the rest of the code.
(Were you using AI? No pressure, I'm not your professor, and I use it a lot in my work as a software dev. However, some things seemed out of place so I thought I'd ask for curiosity sake.)

Anyways, I hope the comments here will be helpful! Please ask me any questions you might have!

src/command-line/execution/executors.cpp

src/command-line/execution/executors.hpp

nguy8tri · 2025-11-04T05:16:58Z

src/command-line/execution/executors.hpp

    /// The Vectorization/Rotation Algorithm being used
    std::unique_ptr<VectorGenerationAlgorithm> vectorizationAlgorithm;
+    /// Optional edge-filtering pipeline (may be empty)
+    EdgeFilteringAlgorithms filters_;


Make std::unique_ptr and reorder

nguy8tri · 2025-11-04T05:17:24Z

src/command-line/parsing/options.hpp

 FOUND_CLI_OPTION("isdda-pdf-order"         , int               , ISDDAPdfOrd     , 2                          , atoi(optarg)               , kNoDefaultArgument, REQ_ASSIGN, "The Probability Density Function Order for ISSDA (even int)"  ) \
 FOUND_CLI_OPTION("isdda-radius-loss-order" , int               , ISDDARadLossOrd , 4                          , atoi(optarg)               , kNoDefaultArgument, REQ_ASSIGN, "The Radius Loss Order ISSDA (even int)"                       ) \
 FOUND_CLI_OPTION("output-file"             , std::string       , outputFile      , ""                         , optarg                     , kNoDefaultArgument, REQ_ASSIGN, "The output file (*.found)"                                    ) \
+FOUND_CLI_OPTION("enable-noop-edge-filter" , bool              , enableNoOpEdgeFilter, false                  , found::strtobool(optarg)   , true              , OPT_ASSIGN, "Enable the NoOp edge filter (test/demo only)"                 ) \


Add a note to remove this upon implementation of the first edge filtering algorithm. Since we do not use NoOp Algorithms, we should not encourage this pattern.

nguy8tri · 2025-11-04T05:22:31Z

src/common/pipeline/pipelines.hpp

        for (size_t i = 0; i < this->size; i++) {
-            dynamic_cast<ModifyingStage<T> *>(this->stages[i])->SetResource(*this->product);
+            auto *stage = static_cast<ModifyingStage<T> *>(this->stages[i]);
+            assert(stage != nullptr);


This check, although admirable, is impractical in nominal use cases, please get rid of the assertion and go straight for the SetResource operation (also meaning you don't need to store a variable)

I'm also interested to hear why you chose a static_cast instead of dynamic_cast. I think it could be justified, just as long as we also agree on it.

I believe I did this mainly just because of testing in pipelines and just put an assert there to make sure it was fine. Forgot to change it back later (I thought it may have been an issue with polymorphism or such).

test/distance/edge-filters-test.cpp

nguy8tri · 2025-11-04T06:34:31Z

test/distance/edge-filters-test.cpp

+}
+
+
+TEST(EdgeFiltersTest, ProvideEdgeFilteringAlgorithm_OptionsDisabledReturnsNullopt) {


No direct invocations of Provide* or Create* in your test cases. These are Dependency Injection functions, and should indirectly invoked via the integration tests.

src/distance/edge-filters.hpp

…s, moving providers to stage_providers, removing autos (replacing w/ std::unique_ptr), reordering distancepipeline

nguy8tri

Good job on the changes. However there were a few changes thay weren't done, so I've explicitly marked them. Let me know if you have questions

nguy8tri · 2025-11-09T01:42:31Z

src/command-line/execution/executors.cpp

+    this->edgeDetectionAlgorithm = std::move(edgeDetectionAlgorithm);
+    this->distanceAlgorithm = std::move(distanceAlgorithm);
+    this->vectorizationAlgorithm = std::move(vectorizationAlgorithm);
+    // Build the pipeline: edge detection -> filters (modifying Points) -> distance -> vectorize


Get rid of comment and put in proper order

nguy8tri · 2025-11-09T01:43:30Z

src/command-line/execution/executors.hpp

+     * @param options The DistanceOptions to configure the pipeline (moved into the executor)
+     * @param edgeDetectionAlgorithm The EdgeDetectionAlgorithm used by the pipeline (moved into the executor)
+     * @param distanceAlgorithm The DistanceDeterminationAlgorithm used by the pipeline (moved into the executor)
+     * @param vectorizationAlgorithm The VectorGenerationAlgorithm used by the pipeline (moved into the executor)


Where did the preconditions go?

nguy8tri · 2025-11-09T01:43:50Z

src/command-line/execution/executors.hpp

+     * @param filters A pipeline of edge filtering stages; ownership is transferred to the executor
+     * @param distanceAlgorithm The DistanceDeterminationAlgorithm used by the pipeline (moved into the executor)
+     * @param vectorizationAlgorithm The VectorGenerationAlgorithm used by the pipeline (moved into the executor)
     */


Same note, where are my preconditions

nguy8tri · 2025-11-09T01:44:54Z

src/distance/edge-filters.hpp

+ public:
+    EdgeFilteringAlgorithm() = default;
+    virtual ~EdgeFilteringAlgorithm() = default;
+    // Implementations override void Run(Points &)


This is understood remove comment

nguy8tri · 2025-11-09T01:46:35Z

src/providers/factory.hpp

+    std::unique_ptr<EdgeDetectionAlgorithm> edgeAlg = ProvideEdgeDetectionAlgorithm(options);
+    std::unique_ptr<DistanceDeterminationAlgorithm> distAlg = ProvideDistanceDeterminationAlgorithm(options);
+    std::unique_ptr<VectorGenerationAlgorithm> vecAlg = ProvideVectorGenerationAlgorithm(options);
+    std::unique_ptr<EdgeFilteringAlgorithms> filtersOpt = ProvideEdgeFilteringAlgorithm(options);


nguy8tri · 2025-11-09T02:12:21Z

src/providers/stage-providers.hpp

+ * used by providers when a no-op filter is requested. This will not be needed
+ * once an EdgeFilteringAlgorithm is implemented.
+ */
+class NoOpEdgeFilter : public ModifyingStage<Points> {


This is in the wrong file, please put this back at the edge-filter file

nguy8tri · 2025-11-09T02:13:10Z

src/providers/stage-providers.hpp

+ * Internal provider instances and singletons used by the stage provider
+ * functions
+ */
+namespace detail {


I should've been explicit here, but unless there is a point, never make another namespace

nguy8tri · 2025-11-09T02:13:24Z

src/providers/stage-providers.hpp

+ * A single shared instance of the NoOpEdgeFilter used by providers when the
+ * no-op edge filter is requested. Documented to satisfy Doxygen checks.
+ */
+inline NoOpEdgeFilter kNoOpEdgeFilter{};


Should be constructed in provider function

nguy8tri · 2025-11-09T02:16:01Z

test/common/mocks/distance-mocks.hpp

    MOCK_METHOD(PositionVector, Run, (const PositionVector& points), (override));
 };

+class MockEdgeFilter : public ModifyingStage<Points> {


Redundant class

nguy8tri · 2025-11-09T02:17:53Z

test/distance/edge-filters-test.cpp

+
+namespace found {
+
+TEST(EdgeFiltersTest, RunOnEmptyThrows) {


Revaluate which test need to be here. Are we testing the behavior of the pipleine/stage or are we testing the behavior of our actual algorithm. Try removing these one by one to see which ones you need.

…, updating pipelines to own stages (and modifying executors/pipeline tests accordingly), updating documentation

MahirEmran · 2025-11-20T21:59:45Z

I had to make a merge in executors-test.cpp because (maybe some commit between last commit and now) there was difference in how DistanceOptions obj was being constructed. I feel like the one that was there before was odd (set all the fields in constructor rather than initializing object, then explicitly setting fields after), so I just went with current changes.

Had to rerun documentation check because first check failed the juliandate time test both attempts.

MahirEmran added 4 commits October 28, 2025 12:14

Adding the algorihtm superclass

63e64b8

more changes - mainly implementing OPT_ASSIGN

719dbbf

first attempt at fixing coverage (still not complete)

5c6d4a5

One function call isn't owrking in pipelines.hpp, not sure why

b2a7b9b

nguy8tri requested changes Nov 4, 2025

View reviewed changes

nguy8tri reviewed Nov 4, 2025

View reviewed changes

src/distance/edge-filters.hpp Outdated Show resolved Hide resolved

nguy8tri and others added 5 commits November 4, 2025 11:02

Fixed Coverage

59a98e7

changing tests to use mocks, adding integration tests for edge filter…

35f4729

…s, moving providers to stage_providers, removing autos (replacing w/ std::unique_ptr), reordering distancepipeline

Update pipelines.hpp

db0e9aa

Forgot some comments

8fed43f

Ignoring a line for coverage in pipelines.hpp

0351c0b

MahirEmran requested a review from nguy8tri November 5, 2025 22:54

nguy8tri changed the title ~~Implementing Algorithm Superclass~~ Feature: Implementing Algorithm Superclass Nov 9, 2025

nguy8tri requested changes Nov 9, 2025

View reviewed changes

MahirEmran and others added 2 commits November 20, 2025 13:33

Removing unneeded tests in edge-filters-test, removing redundant mock…

04b4265

…, updating pipelines to own stages (and modifying executors/pipeline tests accordingly), updating documentation

Merge branch 'main' into algorithm-superclass

0a7de4b

MahirEmran requested a review from nguy8tri November 20, 2025 22:00

		}


		TEST(EdgeFiltersTest, ProvideEdgeFilteringAlgorithm_OptionsDisabledReturnsNullopt) {

Feature: Implementing Algorithm Superclass #47

Are you sure you want to change the base?

Feature: Implementing Algorithm Superclass #47

Uh oh!

Conversation

MahirEmran commented Oct 28, 2025 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Artifacts for PR #47 (DO NOT CHANGE)

Uh oh!

nguy8tri commented Nov 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nguy8tri left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nguy8tri left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MahirEmran commented Nov 20, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

MahirEmran commented Oct 28, 2025 •

edited by github-actions bot

Loading

nguy8tri commented Nov 4, 2025 •

edited

Loading

nguy8tri left a comment •

edited

Loading