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.

Sign up for GitHub

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

[Validate] add warning for unspecified arguments, and change default values #277

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

[Validate] add warning for unspecified arguments, and change default values #277

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2e373ac
add warning for unspecified arguments, and change default values
sasha-scale Apr 19, 2022
5f726e2
add unit test coveragE
sasha-scale Apr 19, 2022
af6c06e
address PR comments
sasha-scale Apr 23, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions nucleus/metrics/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@
)
from nucleus.prediction import PredictionList

EPSILON = 10 ** -4 # 0.0001


class MetricResult(ABC):
"""Base MetricResult class"""
Expand Down Expand Up @@ -41,6 +43,14 @@ def aggregate(results: Iterable["ScalarResult"]) -> "ScalarResult":
value = total_value / max(total_weight, sys.float_info.epsilon)
return ScalarResult(value, total_weight)

def __eq__(self, other):
if not isinstance(other, self.__class__):
return False
return (
abs(self.value - other.value) < EPSILON
and self.weight == other.weight
)


class Metric(ABC):
"""Abstract class for defining a metric, which takes a list of annotations
Expand Down
52 changes: 41 additions & 11 deletions nucleus/metrics/cuboid_metrics.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import sys
import warnings
from abc import abstractmethod
from typing import List, Optional, Union

Expand All @@ -10,6 +11,9 @@
from .filtering import ListOfAndFilters, ListOfOrAndFilters
from .filters import confidence_filter

DEFAULT_IOU_THRESHOLD = 0.1
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we changing the default from 0.0 to 0.1? I'm not against it would just be nice to have a comment explaining the choice of the default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the default of 0.0 is really confusing in my opinion, in particular because it makes predictions with zero overlap true positives. 0.1 seemed reasonable for 3D IoU, though I wouldn't be opposed to 0.5 (to match the 2D default threshold) - what do you think?

DEFAULT_CONFIDENCE_THRESHOLD = 0.0


class CuboidMetric(Metric):
"""Abstract class for metrics of cuboids.
Expand All @@ -28,7 +32,7 @@ class CuboidMetric(Metric):
def __init__(
self,
enforce_label_match: bool = False,
confidence_threshold: float = 0.0,
confidence_threshold: Optional[float] = None,
Copy link
Contributor

Choose a reason for hiding this comment

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

doesn't it make sense to also add the IOU threshold to the base class? How would a metric make sense without computing the IOU for the matches?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also noticed that the iou_threshold wasn't used in the base class, I can add

annotation_filters: Optional[
Union[ListOfOrAndFilters, ListOfAndFilters]
] = None,
Expand All @@ -54,6 +58,11 @@ def __init__(
(AND), forming a more selective and multiple column predicate. Finally, the most outer list combines
these filters as a disjunction (OR).
"""
if not confidence_threshold:
confidence_threshold = DEFAULT_CONFIDENCE_THRESHOLD
warnings.warn(
f"Got confidence_threshold value of `None`. In this case, we set the confidence_threshold to {confidence_threshold} (include all predictions, regardless of confidence). Consider specifying this value explicitly during metric initialization"
)
self.enforce_label_match = enforce_label_match
assert 0 <= confidence_threshold <= 1
self.confidence_threshold = confidence_threshold
Expand Down Expand Up @@ -99,8 +108,8 @@ class CuboidIOU(CuboidMetric):
def __init__(
self,
enforce_label_match: bool = True,
iou_threshold: float = 0.0,
confidence_threshold: float = 0.0,
iou_threshold: Optional[float] = None,
confidence_threshold: Optional[float] = None,
iou_2d: bool = False,
annotation_filters: Optional[
Union[ListOfOrAndFilters, ListOfAndFilters]
Expand All @@ -127,6 +136,11 @@ def __init__(
interpreted as a conjunction (AND), forming a more selective and multiple column predicate.
Finally, the most outer list combines these filters as a disjunction (OR).
"""
if not iou_threshold:
iou_threshold = DEFAULT_IOU_THRESHOLD
warnings.warn(
f"The IoU threshold used for matching was initialized to `None`. In this case, the value of iou_threshold defaults to {iou_threshold}. If this values will produce unexpected behavior, consider specifying the iou_threshold argument during metric initialization"
)
assert (
0 <= iou_threshold <= 1
), "IoU threshold must be between 0 and 1."
Expand All @@ -147,13 +161,15 @@ def eval(
iou_3d_metric, iou_2d_metric = detection_iou(
predictions,
annotations,
threshold_in_overlap_ratio=self.iou_threshold,
self.iou_threshold,
self.enforce_label_match,
)

weight = max(len(annotations), len(predictions))
if self.iou_2d:
weight = len(iou_2d_metric)
avg_iou = iou_2d_metric.sum() / max(weight, sys.float_info.epsilon)
else:
weight = len(iou_3d_metric)
avg_iou = iou_3d_metric.sum() / max(weight, sys.float_info.epsilon)
Copy link
Contributor

Choose a reason for hiding this comment

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

if the weight is zero by some chance (is that even possible?), won't that make avg_iou absurdly large since we would dividing by sys.float_info.epsilon?

Perhaps if weight == 0 then avg_iou should be 0 too?

Not sure what is correct here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch - I agree with you the boundary condition is incorrect here, the iou should be zero with weight zero in that case

I will also add unit test coverage for this case


return ScalarResult(avg_iou, weight)
Expand All @@ -166,8 +182,8 @@ class CuboidPrecision(CuboidMetric):
def __init__(
self,
enforce_label_match: bool = True,
iou_threshold: float = 0.0,
confidence_threshold: float = 0.0,
iou_threshold: Optional[float] = None,
confidence_threshold: Optional[float] = None,
annotation_filters: Optional[
Union[ListOfOrAndFilters, ListOfAndFilters]
] = None,
Expand All @@ -192,6 +208,11 @@ def __init__(
interpreted as a conjunction (AND), forming a more selective and multiple column predicate.
Finally, the most outer list combines these filters as a disjunction (OR).
"""
if not iou_threshold:
iou_threshold = DEFAULT_IOU_THRESHOLD
warnings.warn(
f"The IoU threshold used for matching was initialized to `None`. In this case, the value of iou_threshold defaults to {iou_threshold}. If this values will produce unexpected behavior, consider specifying the iou_threshold argument during metric initialization"
)
assert (
0 <= iou_threshold <= 1
), "IoU threshold must be between 0 and 1."
Expand All @@ -211,7 +232,9 @@ def eval(
stats = recall_precision(
predictions,
annotations,
threshold_in_overlap_ratio=self.iou_threshold,
self.iou_threshold,
self.confidence_threshold,
self.enforce_label_match,
)
weight = stats["tp_sum"] + stats["fp_sum"]
precision = stats["tp_sum"] / max(weight, sys.float_info.epsilon)
Expand All @@ -225,8 +248,8 @@ class CuboidRecall(CuboidMetric):
def __init__(
self,
enforce_label_match: bool = True,
iou_threshold: float = 0.0,
confidence_threshold: float = 0.0,
iou_threshold: Optional[float] = None,
confidence_threshold: Optional[float] = None,
Comment on lines +254 to +255
Copy link
Contributor

Choose a reason for hiding this comment

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

Changes to default params needs to be reflected in nucleus/validate/eval_functions/available_eval_functions.py configurations as well since that is the client facing code. The defaults there override the defaults provided here if not reflected.

annotation_filters: Optional[
Union[ListOfOrAndFilters, ListOfAndFilters]
] = None,
Expand All @@ -241,6 +264,11 @@ def __init__(
iou_threshold: IOU threshold to consider detection as valid. Must be in [0, 1]. Default 0.0
confidence_threshold: minimum confidence threshold for predictions. Must be in [0, 1]. Default 0.0
"""
if not iou_threshold:
iou_threshold = DEFAULT_IOU_THRESHOLD
warnings.warn(
f"The IoU threshold used for matching was initialized to `None`. In this case, the value of iou_threshold defaults to {iou_threshold}. If this values will produce unexpected behavior, consider specifying the iou_threshold argument during metric initialization"
)
assert (
0 <= iou_threshold <= 1
), "IoU threshold must be between 0 and 1."
Expand All @@ -260,7 +288,9 @@ def eval(
stats = recall_precision(
predictions,
annotations,
threshold_in_overlap_ratio=self.iou_threshold,
self.iou_threshold,
self.confidence_threshold,
self.enforce_label_match,
)
weight = stats["tp_sum"] + stats["fn_sum"]
recall = stats["tp_sum"] / max(weight, sys.float_info.epsilon)
Expand Down
31 changes: 25 additions & 6 deletions nucleus/metrics/cuboid_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,18 +101,25 @@ def wrapper(
return wrapper


def process_dataitem(dataitem):
def process_dataitem(item_list, confidence_threshold=None):
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a pretty generic name that could be more concrete. If you have the context it would be nice to re-name or add a docstring to explain what this "processing" is doing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't really have much context but I agree with you this is a terrible name... I will try to make something better

if confidence_threshold:
item_list = [
item
for item in item_list
if item.confidence >= confidence_threshold
]
processed_item = {}
processed_item["xyz"] = np.array(
[[ann.position.x, ann.position.y, ann.position.z] for ann in dataitem]
[[ann.position.x, ann.position.y, ann.position.z] for ann in item_list]
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we actually have a dict here for the processed item? Wouldn't a dataclass or namedtuple. I'm not a huge fan of dicts interface wise.

)
processed_item["wlh"] = np.array(
[
[ann.dimensions.x, ann.dimensions.y, ann.dimensions.z]
for ann in dataitem
for ann in item_list
]
)
processed_item["yaw"] = np.array([ann.yaw for ann in dataitem])
processed_item["yaw"] = np.array([ann.yaw for ann in item_list])
processed_item["labels"] = [ann.label for ann in item_list]
return processed_item


Expand Down Expand Up @@ -278,6 +285,8 @@ def recall_precision(
prediction: List[CuboidPrediction],
groundtruth: List[CuboidAnnotation],
threshold_in_overlap_ratio: float,
confidence_threshold: float,
enforce_label_match: bool,
) -> Dict[str, float]:
"""
Calculates the precision and recall of each lidar frame.
Expand All @@ -295,7 +304,7 @@ def recall_precision(
num_instances = 0

gt_items = process_dataitem(groundtruth)
pred_items = process_dataitem(prediction)
pred_items = process_dataitem(prediction, confidence_threshold)

num_predicted += pred_items["xyz"].shape[0]
num_instances += gt_items["xyz"].shape[0]
Expand All @@ -317,6 +326,10 @@ def recall_precision(
for pred_id, gt_id in mapping:
if fn[gt_id] == 0:
continue
if enforce_label_match and not (
gt_items["labels"][gt_id] == pred_items["labels"][pred_id]
):
continue
tp[pred_id] = 1
fp[pred_id] = 0
fn[gt_id] = 0
Expand All @@ -340,6 +353,7 @@ def detection_iou(
prediction: List[CuboidPrediction],
groundtruth: List[CuboidAnnotation],
threshold_in_overlap_ratio: float,
enforce_label_match: bool,
) -> Tuple[np.ndarray, np.ndarray]:
"""
Calculates the 2D IOU and 3D IOU overlap between predictions and groundtruth.
Expand Down Expand Up @@ -370,8 +384,13 @@ def detection_iou(
)

for i, m in enumerate(iou_3d.max(axis=1)):
j = iou_3d[i].argmax()
if (
enforce_label_match
and gt_items["labels"][i] != pred_items["labels"][j]
):
continue
if m >= threshold_in_overlap_ratio:
j = iou_3d[i].argmax()
meter_3d.append(iou_3d[i, j])
meter_2d.append(iou_2d[i, j])

Expand Down
Loading