Copilot commented on code in PR #58:
URL: https://github.com/apache/solr-orbit/pull/58#discussion_r3661314974
##########
solrorbit/aggregator.py:
##########
@@ -222,23 +221,44 @@ def calculate_weighted_average(self, task_metrics:
Dict[str, List[Any]], task_na
if metric_field == 'unit':
weighted_metrics[metric][metric_field] =
values[0][metric_field]
elif metric_field == 'min':
- weighted_metrics[metric]['overall_min'] =
min(value.get(metric_field, 0) for value in values)
+ item_values = [value.get(metric_field) for value in
values]
+ weighted_metrics[metric]['overall_min'] = min(
+ (value for value in item_values if value is not
None), default=None)
elif metric_field == 'max':
- weighted_metrics[metric]['overall_max'] =
max(value.get(metric_field, 0) for value in values)
+ item_values = [value.get(metric_field) for value in
values]
+ weighted_metrics[metric]['overall_max'] = max(
+ (value for value in item_values if value is not
None), default=None)
else:
# for items like median or containing percentile values
- item_values = [value.get(metric_field, 0) for value in
values]
- weighted_sum = sum(value * iterations for value,
iterations in zip(item_values, iterations_per_run))
- weighted_metrics[metric][metric_field] = weighted_sum
/ total_iterations
+ item_values = [value.get(metric_field) for value in
values]
+ weighted_metrics[metric][metric_field] =
self.weighted_mean(item_values, iterations_per_run)
else:
- weighted_sum = sum(value * iterations for value, iterations in
zip(values, iterations_per_run))
- weighted_metrics[metric] = weighted_sum / total_iterations
+ weighted_metrics[metric] = self.weighted_mean(values,
iterations_per_run)
return weighted_metrics
+ @staticmethod
+ def weighted_mean(values: List[Any], iterations_per_run: List[int]) -> Any:
+ """
+ Weights each test run's value by that run's iteration count.
+ Operations that produced no valid samples report their metrics as
None; those are left out
+ of both the sum and the divisor, and the result is None when no run
contributed a value,
+ so that "not measured" stays distinct from a measured zero
+ """
+ contributions = [(value, iterations) for value, iterations in
zip(values, iterations_per_run)
+ if value is not None]
+ total_iterations = sum(iterations for _, iterations in contributions)
+ if not total_iterations:
+ return None
+ return sum(value * iterations for value, iterations in contributions)
/ total_iterations
+
def calculate_rsd(self, values: List[Union[int, float]], metric_name: str)
-> Union[float, str]:
if not values:
raise ValueError(f"Cannot calculate RSD for metric
'{metric_name}': empty list of values")
+ # operations that produced no valid samples report None, which cannot
contribute to a deviation
+ values = [value for value in values if value is not None]
+ if not values:
Review Comment:
`calculate_rsd` now explicitly filters out `None` values, but its type
signature still claims the input list contains only `int`/`float`. Updating the
annotation avoids misleading callers and type-checkers.
##########
solrorbit/aggregator.py:
##########
@@ -222,23 +221,44 @@ def calculate_weighted_average(self, task_metrics:
Dict[str, List[Any]], task_na
if metric_field == 'unit':
weighted_metrics[metric][metric_field] =
values[0][metric_field]
elif metric_field == 'min':
- weighted_metrics[metric]['overall_min'] =
min(value.get(metric_field, 0) for value in values)
+ item_values = [value.get(metric_field) for value in
values]
+ weighted_metrics[metric]['overall_min'] = min(
+ (value for value in item_values if value is not
None), default=None)
elif metric_field == 'max':
- weighted_metrics[metric]['overall_max'] =
max(value.get(metric_field, 0) for value in values)
+ item_values = [value.get(metric_field) for value in
values]
+ weighted_metrics[metric]['overall_max'] = max(
+ (value for value in item_values if value is not
None), default=None)
else:
# for items like median or containing percentile values
- item_values = [value.get(metric_field, 0) for value in
values]
- weighted_sum = sum(value * iterations for value,
iterations in zip(item_values, iterations_per_run))
- weighted_metrics[metric][metric_field] = weighted_sum
/ total_iterations
+ item_values = [value.get(metric_field) for value in
values]
+ weighted_metrics[metric][metric_field] =
self.weighted_mean(item_values, iterations_per_run)
else:
- weighted_sum = sum(value * iterations for value, iterations in
zip(values, iterations_per_run))
- weighted_metrics[metric] = weighted_sum / total_iterations
+ weighted_metrics[metric] = self.weighted_mean(values,
iterations_per_run)
return weighted_metrics
+ @staticmethod
+ def weighted_mean(values: List[Any], iterations_per_run: List[int]) -> Any:
Review Comment:
`weighted_mean` currently uses `List[Any]` / return `Any`, but the
implementation returns a numeric weighted average or `None` and assumes values
are numeric (it does `value * iterations`). Tightening the type hints makes
intent clearer and helps static analysis catch accidental non-numeric inputs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
