implemented label in cluster estimation #231

Aleksa-M · 2024-11-29T22:24:14Z

No description provided.

maxlou05

Reviewed

modules/cluster_estimation/cluster_estimation.py

maxlou05

Reviewed

modules/cluster_estimation/cluster_estimation.py

maxlou05

Reviewed

modules/cluster_estimation/cluster_estimation.py

Xierumeng

I skimmed the code and it looks very complect. My suggestion is to use composition: Leave the original class alone, and create a new class (e.g. ClusterEstimationByLabel or something). This new class contains a dictionary of label to cluster estimation object, and is responsible for sorting the incoming points into the correct object in the dictionary and collecting the individual return values, which it then returns to the worker.

modules/cluster_estimation/cluster_estimation.py

maxlou05

Reviewed

maxlou05 · 2025-01-31T01:39:13Z

modules/cluster_estimation/cluster_estimation.py

@@ -277,7 +288,7 @@ def __sort_by_weights(
    @staticmethod
    def __convert_detections_to_point(
        detections: "list[detection_in_world.DetectionInWorld]",
-    ) -> "list[tuple[float, float]]":
+    ) -> "list[tuple[float, float, int]]":


Why do we change this function signature? You never changed the function so it still returns a (float, float)

modules/cluster_estimation/cluster_estimation_by_label.py

maxlou05 · 2025-01-31T01:55:45Z

modules/cluster_estimation/cluster_estimation_by_label.py

+        if min_activation_threshold < 1:
+            return False, None
+
+        return True, ClusterEstimationByLabel(


Please check ClusterEstimation's restrictions. Either apply them again here or by invoking creating cluster estimation

maxlou05 · 2025-01-31T02:10:29Z

modules/cluster_estimation/cluster_estimation_by_label.py

+        labels_to_object_clusters: dict[int, list[object_in_world.ObjectInWorld] or None.
+            Dictionary where the key is a label and the value is a list of all cluster detections with that label
+        """
+        label_to_detections: dict[int, list[detection_in_world.DetectionInWorld]] = {}


Add comment saying sorting detections by label

modules/cluster_estimation/cluster_estimation_worker.py

maxlou05 · 2025-01-31T02:47:43Z

tests/unit/test_cluster_detection.py

+        self, cluster_model_by_label: cluster_estimation_by_label.ClusterEstimationByLabel
+    ) -> None:
+        """
+        Five clusters with small standard devition that have different labels


Edit doc string

maxlou05 · 2025-01-31T02:48:35Z

tests/unit/test_cluster_detection.py

+            1: [100, 100, 100],
+            2: [100, 100, 100],
+            3: [100, 100, 100],


Can you make each point different weights and different number of clusters, so it's a little more random?

do weight and confidence mean the same thing?

maxlou05 · 2025-01-31T02:52:50Z

tests/unit/test_cluster_detection.py

+        Five clusters with small standard devition that all have the same label
+        """
+        # Setup
+        labels_to_n_samples_per_cluster = {1: [100, 100, 100, 100, 100]}


Can you test this with different number of points per cluster?

maxlou05 · 2025-01-31T03:03:55Z

tests/unit/test_cluster_detection.py

@@ -488,3 +566,97 @@ def test_position_regular_data(
                    break

            assert is_match
+
+
+class TestCorrectClusterEstimationByLabel:


Maybe test some error cases, like what happens if things violate your input conditions? For example, what happens when only 1 new point is passed to your Cluster Estimation By Label, it still runs. However, Cluster Estimation has a higher min_new_points, so nothing ever gets run and that point is just lost?

Xierumeng

Reviewed.

modules/cluster_estimation/cluster_estimation.py

modules/cluster_estimation/cluster_estimation_by_label.py

modules/cluster_estimation/cluster_estimation_worker.py

modules/object_in_world.py

tests/unit/test_cluster_detection.py

Xierumeng

Reviewed.

modules/cluster_estimation/cluster_estimation.py

modules/cluster_estimation/cluster_estimation_by_label.py

modules/cluster_estimation/cluster_estimation_worker.py

tests/unit/test_cluster_detection.py

tests/unit/test_cluster_estimation_by_label.py

Xierumeng

Reviewed.

Xierumeng · 2025-03-07T04:18:27Z

modules/cluster_estimation/cluster_estimation.py

+        """
+        Checks if a valid cluster estimation object can be constructed.
+
+        See `ClusterEstimation` for parameter descriptions.


Change this to:

See `create()` for parameter descriptions. Return: Whether the arguments are valid.

Xierumeng · 2025-03-07T04:19:15Z

modules/cluster_estimation/cluster_estimation_by_label.py

+
+    ATTRIBUTES
+    ----------
+    min_activation_threshold: int
+        Minimum total data points before model runs. Must be at least max_num_components.
+
+    min_new_points_to_run: int
+        Minimum number of new data points that must be collected before running model.
+
+    max_num_components: int
+        Max number of real landing pads. Must be at least 1.
+
+    random_state: int
+        Seed for randomizer, to get consistent results.
+
+    local_logger: Logger
+        For logging error and debug messages.
+
+    METHODS
+    -------
+    run()
+        Cluster estimation filtered by label.


Remove this.

Xierumeng · 2025-03-07T04:22:18Z

modules/cluster_estimation/cluster_estimation_by_label.py

+        RETURNS
+        -------
+        model_ran: bool
+            True if ClusterEstimation object successfully ran its estimation model, False otherwise.
+
+        labels_to_objects: dict[int, list[object_in_world.ObjectInWorld] or None.
+            Dictionary where the key is a label and the value is a list of all cluster detections with that label.
+            ObjectInWorld objects don't have a label property, but they are sorted into label categories in the dictionary.


Simplify:

Return: Success, labels and their associated objects.

Xierumeng · 2025-03-07T04:24:59Z

modules/cluster_estimation/cluster_estimation_by_label.py

+
+            if not label in labels_to_objects:
+                labels_to_objects[label] = []
+            labels_to_objects[label] += clusters


Add empty line above this line.

Xierumeng · 2025-03-07T04:28:57Z

modules/cluster_estimation/cluster_estimation_by_label.py

+
+            if not label in labels_to_objects:
+                labels_to_objects[label] = []
+            labels_to_objects[label] += clusters


Is this the desired behaviour? The cluster estimation objects already hold a record of all points, so they will always generate an updated version of the cluster centres. I think this should be an unconditional assignment instead.

Xierumeng · 2025-03-07T04:32:01Z

tests/unit/test_cluster_estimation_by_label.py

+"""
+
+import random
+import numpy as np


Add empty line between system and 3rd party imports.

Xierumeng · 2025-03-07T04:32:28Z

tests/unit/test_cluster_estimation_by_label.py

+
+from modules.cluster_estimation import cluster_estimation_by_label
+from modules.common.modules.logger import logger
+from modules import detection_in_world


Move this above (shorter is higher in alphabetical order).

Xierumeng · 2025-03-07T04:34:04Z

tests/unit/test_cluster_estimation_by_label.py

+from modules.common.modules.logger import logger
+from modules import detection_in_world
+
+MIN_TOTAL_POINTS_THRESHOLD = 100


2 empty lines total between imports and global constants.

Xierumeng · 2025-03-07T04:34:10Z

tests/unit/test_cluster_estimation_by_label.py

+RNG_SEED = 0
+CENTRE_BOX_SIZE = 500
+
+# Test functions use test fixture signature names and access class privates


2 empty lines total above.

Xierumeng · 2025-03-07T04:53:52Z

tests/unit/test_cluster_estimation_by_label.py

Most of this testing is unnecessary and brittle (it will break if the implementation of ClusterEstimation is changed). The goal of these tests is ensuring that the labelled points go to the correct cluster estimation objects. The tests themselves are for various conditions. For example:

Never before seen labelled point (is the cluster estimation object created correctly?)

Existing labelled point goes to the correct cluster estimation object

Multiple points go to the correct places

Non consecutive labels (i.e. label values that skip numbers (e.g. {0, 1, 2, 5} )

Verifying that the points go to the correct objects can be done by accessing the ClusterEstimationByLabel and ClusterEstimation members.

Verifying the outputs is a little trickier, but can be done by making the objects return a number of cluster centres corresponding to their label (e.g. the object at 3 returns 3 centres when run() is called). When constructing them, the objects can be directly modified by creating a huge number of points exactly at the same location at each of the 3 centres, no need for any fancy cluster generation.

There is also no need to check whether the object actually ran with the thresholds either (basically, this test should still pass if someone removes __decide_to_run() from cluster estimation).

maxlou05 reviewed Nov 30, 2024

View reviewed changes

maxlou05 reviewed Dec 21, 2024

View reviewed changes

Aleksa-M force-pushed the cluster-estimation-labels branch from c43b88f to 3134b2a Compare January 7, 2025 02:39

maxlou05 reviewed Jan 18, 2025

View reviewed changes

modules/cluster_estimation/cluster_estimation.py Outdated Show resolved Hide resolved

Xierumeng reviewed Jan 19, 2025

View reviewed changes

modules/cluster_estimation/cluster_estimation.py Outdated Show resolved Hide resolved

maxlou05 reviewed Jan 31, 2025

View reviewed changes

Aleksa-M added 17 commits February 5, 2025 19:29

implemented label in cluster estimation

3205913

implemented cluster estimation label by detection label

dabc8cb

fixed implementation

a101d17

implemented fixes

571cc13

implemented changes, made it pass all tests

61dcfa1

work in progress commit

9c70b00

tests working for cluster by label

0b89706

reformated

f5c4b90

implemented label in cluster estimation

f687063

implemented cluster estimation label by detection label

c0ea3e0

fixed implementation

50ef6ff

implemented fixes

42c9fbe

implemented changes, made it pass all tests

6c8e724

work in progress commit

b702a76

tests working for cluster by label

34224fe

reformated

6c02dc8

integrated review changes

2106bbe

Aleksa-M force-pushed the cluster-estimation-labels branch from d8b63f7 to 2106bbe Compare February 6, 2025 01:08

removed label parameter from default cluster estimation

1692e16

Xierumeng reviewed Feb 7, 2025

View reviewed changes

Cyuber added 2 commits February 11, 2025 01:08

implemented review changes

769ba6e

formatting changes

8618361

Xierumeng reviewed Feb 14, 2025

View reviewed changes

implemented reviewed changes

d3c0c89

Cyuber and others added 2 commits March 5, 2025 23:24

fixed formatting

d12aa34

empty commit to fix contributing account

b8155cb

Xierumeng reviewed Mar 7, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implemented label in cluster estimation #231

implemented label in cluster estimation #231

Aleksa-M commented Nov 29, 2024

maxlou05 left a comment

maxlou05 left a comment

maxlou05 left a comment

Xierumeng left a comment

maxlou05 left a comment

maxlou05 Jan 31, 2025

maxlou05 Jan 31, 2025

maxlou05 Jan 31, 2025

maxlou05 Jan 31, 2025

maxlou05 Jan 31, 2025

Aleksa-M Feb 11, 2025

maxlou05 Jan 31, 2025

maxlou05 Jan 31, 2025

Xierumeng left a comment

Xierumeng left a comment

Xierumeng left a comment

Xierumeng Mar 7, 2025

Xierumeng Mar 7, 2025

Xierumeng Mar 7, 2025 •

edited

Loading

Xierumeng Mar 7, 2025

Xierumeng Mar 7, 2025

Xierumeng Mar 7, 2025

Xierumeng Mar 7, 2025

Xierumeng Mar 7, 2025 •

edited

Loading

Xierumeng Mar 7, 2025 •

edited

Loading

Xierumeng Mar 7, 2025

implemented label in cluster estimation #231

Are you sure you want to change the base?

implemented label in cluster estimation #231

Conversation

Aleksa-M commented Nov 29, 2024

maxlou05 left a comment

Choose a reason for hiding this comment

maxlou05 left a comment

Choose a reason for hiding this comment

maxlou05 left a comment

Choose a reason for hiding this comment

Xierumeng left a comment

Choose a reason for hiding this comment

maxlou05 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Xierumeng left a comment

Choose a reason for hiding this comment

Xierumeng left a comment

Choose a reason for hiding this comment

Xierumeng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Xierumeng Mar 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Xierumeng Mar 7, 2025 • edited Loading

Choose a reason for hiding this comment

Xierumeng Mar 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Xierumeng Mar 7, 2025 •

edited

Loading

Xierumeng Mar 7, 2025 •

edited

Loading

Xierumeng Mar 7, 2025 •

edited

Loading