MONAI support - #33
Conversation
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
Signed-off-by: Eric Kerfoot <17726042+ericspod@users.noreply.github.com>
Updating setup.py to address pkg_resources
WalkthroughChangesMulticlass and binary pairwise metrics now support axis-aware reductions, smoothing parameters, batched skeleton processing, updated empty-input behavior, and formatted metric serialization. Packaging metadata and requirement parsing are also updated. Axis-aware pairwise metrics
Packaging metadata
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant BinaryPairwiseMeasures
participant compute_skeleton
participant compute_center_of_mass
participant MetricOutput
BinaryPairwiseMeasures->>compute_skeleton: request axis-aware skeletons
compute_skeleton-->>BinaryPairwiseMeasures: return batched skeleton array
BinaryPairwiseMeasures->>compute_center_of_mass: calculate center of mass
compute_center_of_mass-->>BinaryPairwiseMeasures: return coordinates or -1
BinaryPairwiseMeasures->>MetricOutput: format computed metric groups
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
⚔️ Resolve merge conflicts 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
MetricsReloaded/metrics/pairwise_measures.py (2)
912-928: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
n_intersection()is not axis-aware, so these ratios break under batching.n_pos_ref()/n_union()now reduce overself.axis(per-sample vector) whilen_intersection()still sums the whole array (scalar), yielding a scalar-numerator / vector-denominator mix. Maken_intersectionreduce overself.axistoo.🐛 Suggested fix (in `n_intersection`)
- return np.sum(self.__intersection_map()) + return np.sum(self.__intersection_map(), axis=self.axis)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@MetricsReloaded/metrics/pairwise_measures.py` around lines 912 - 928, Update n_intersection() to reduce its inputs over self.axis, matching the axis-aware behavior of n_pos_ref() and n_union(). Preserve its existing intersection calculation while returning per-sample values so intersection_over_union() and the related ratio no longer mix scalar numerators with vector denominators.
555-576: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftScalar empty-case guards defeat the new batch support. When
axisexcludes a batch dimension these reductions return arrays, soself.n_pos_ref() == 0/np.isnan(denominator)become element-wise arrays andifraisesValueError: truth value of an array ... is ambiguous. Same pattern inrecall(762, 765),fbeta(822, 827),negative_predictive_values(869), anddsc(787). Considernp.where-based masking (with awarnings.warnwhennp.any(...)) so both scalar and batched inputs work.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@MetricsReloaded/metrics/pairwise_measures.py` around lines 555 - 576, Update the empty-case guards in specificity, recall, fbeta, negative_predictive_values, and dsc to support scalar and batched reductions when axis excludes a batch dimension. Replace scalar if checks on array-valued counts or denominators with element-wise masking via np.where, and emit the existing warning when np.any of the invalid-case mask is true while preserving nan results for invalid elements.
🧹 Nitpick comments (7)
setup.py (2)
24-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the setup-time debug print.
setup.pywill print the dependency list on every build or installation, polluting package-manager output. Delete this statement.Proposed fix
-print(requirements)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@setup.py` at line 24, Remove the print(requirements) debug statement from setup.py, leaving dependency handling and the rest of the setup flow unchanged.
53-55: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winValidate the newly advertised Python versions in CI.
The package now advertises Python 3.11-3.13, but the provided workflow only tests Python 3.9. Expand the CI matrix or verify compatibility before publishing these classifiers.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@setup.py` around lines 53 - 55, Update the CI workflow’s Python test matrix to run the versions advertised by the classifiers in setup.py, especially Python 3.11–3.13, or remove any unsupported classifiers until compatibility is verified. Preserve existing supported-version coverage and ensure the published Python version metadata matches what CI validates.MetricsReloaded/metrics/pairwise_measures.py (2)
128-133: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
ref_numbusesone_hot_pred.shapeto pick its reduction axis. Works only while pred and ref have identical rank; useone_hot_ref.shapefor clarity/safety.♻️ Proposed tweak
- ref_numb = np.sum(one_hot_ref, axis=len(one_hot_pred.shape) - 2) + ref_numb = np.sum(one_hot_ref, axis=len(one_hot_ref.shape) - 2)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@MetricsReloaded/metrics/pairwise_measures.py` around lines 128 - 133, Update the ref_numb reduction in the pairwise measure calculation to derive its axis from one_hot_ref.shape rather than one_hot_pred.shape, while leaving pred_numb and the subsequent matrix multiplication unchanged.
96-98: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
is Nonefor the sentinel check. Ruff flags E711 here (and at line 359).♻️ Proposed tweak
- if self.axis == None: + if self.axis is None: self.axis = (0, 1)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@MetricsReloaded/metrics/pairwise_measures.py` around lines 96 - 98, Update the axis sentinel checks in the relevant initialization logic and the code at line 359 from equality comparison with None to identity comparison using is None, preserving the existing default-axis behavior.Source: Linters/SAST tools
test/test_metrics/test_pairwise_measures.py (1)
387-400: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo coverage for the new
axis/smooth_drbehaviour. All tests still exercise unbatched inputs withsmooth_dr=0, so the batched reductions and smoothing paths added in this PR are untested — which is where the scalar guards and skeleton batching break. Consider adding a case with a leading batch dim (e.g.PM(pred[None], ref[None], axis=(1, 2))) and one withsmooth_dr>0.Also,
measured_hausdorff_distance_perc()on all-empty input triggers NumPy's "All-NaN slice encountered" RuntimeWarning; asserting it viapytest.warnswould make the intent explicit and keep the test green under-W error.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/test_metrics/test_pairwise_measures.py` around lines 387 - 400, Expand test_distance_empty to cover batched inputs by constructing PM with a leading batch dimension and axis=(1, 2), and add coverage for a positive smooth_dr value while preserving the expected empty-distance results. Wrap the all-empty measured_hausdorff_distance_perc() assertion with pytest.warns for NumPy’s RuntimeWarning, then assert the NaN result so the warning is intentional under -W error.MetricsReloaded/utility/utils.py (2)
282-290: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
axes[0]as a proxy for "number of leading dims" is fragile. It silently assumesaxesis sorted and that spatial axes are trailing and contiguous; e.g.axes=(0, 2)would giveiter_ix=[]and skeletonize the whole array. Consider validating (axes == tuple(range(axes[0], ndim))) or taking an explicitn_leading_dimsargument. Also,skeletonizeexpects a binary image andnp.zeros_like(img)keeps the input dtype, so float inputs will hold cast booleans — worth asserting/normalising.Style: Ruff flags E701 on line 283 (
if axes is None: axes = [0]) — put the assignment on its own line and useis None.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@MetricsReloaded/utility/utils.py` around lines 282 - 290, Update the surrounding skeletonization logic to avoid inferring leading-dimension count solely from axes[0]: validate that axes represent the expected contiguous trailing spatial axes or derive the count explicitly, and reject unsupported layouts such as (0, 2). Normalize or validate img as binary and ensure the skeleton output uses an appropriate boolean-compatible dtype. In the same block, split the axes None assignment onto its own line to resolve Ruff E701.Source: Linters/SAST tools
315-317: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSentinel return type differs from the success path. Returning
-1(int) vs a coordinate tuple pushes type-checking onto every caller;com_distalready duplicates the emptiness check. Acceptable given the tests assert it, butNoneornp.nan-filled coordinates would be cleaner.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@MetricsReloaded/utility/utils.py` around lines 315 - 317, Update the center-of-mass helper’s empty-image branch to return a coordinate-shaped sentinel consistent with ndimage.center_of_mass, such as NaN-filled coordinates, instead of integer -1. Adjust com_dist only as needed to recognize that sentinel while preserving the existing empty-image behavior and tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@MetricsReloaded/metrics/pairwise_measures.py`:
- Around line 899-903: Update fppi to sum false positives across the spatial
axes before averaging across the image or batch axis, so it returns false
positives per image rather than the overall binary-map rate. Preserve the
existing __fp_map() source and axis configuration while separating spatial
reduction from image-level averaging.
- Around line 249-255: Update the batched reduction logic in the affected
pairwise metric method and expected_cost to sum only the confusion-matrix axes,
using the final two dimensions for 3D inputs while preserving the configured
axis for unbatched inputs. Ensure batched results retain one value per batch
instead of collapsing the batch dimension.
- Around line 1174-1183: Update the Hausdorff-distance calculation around
border_distance() to copy ref_border_dist and pred_border_dist before applying
the ~msk NaN mask, preventing mutation of the cached arrays while preserving the
existing percentile reduction and return behavior.
- Around line 640-648: Update the expected calibration calculation around the
ecn assignments to replace boolean-mask indexing with np.where-based selection,
preserving the alpha >= 1 and alpha < 1 formulas while supporting both NumPy
scalar values from fully reduced axes and batched arrays.
- Around line 1196-1243: The serializers to_string_count, to_string_dist, and
to_string_mt reference counting_dict, distance_dict, and multi_thresholds_dict
without ensuring those dictionaries are populated. Initialize and populate these
metric dictionaries consistently with measures_count, measures_dist, and
measures_mthresh before serialization, or remove the affected serializer methods
if the data is intentionally unsupported.
- Around line 883-897: Update dice_score to set or validate the "beta" option
consumed by fbeta(), not "fbeta", without mutating shared default configuration
state; ensure each instance owns its own dict_args before applying the
dice-specific value. Remove the debug print from the already-correct branch
while preserving the warning when overriding a non-1 beta.
In `@MetricsReloaded/utility/utils.py`:
- Around line 291-294: Fix the single-dimension branch in the image
skeletonization loop so it iterates over the valid indices of the leading
dimension rather than the integer value of img.shape[0]. Preserve the existing
per-slice assignment to skeleton using skeletonize(img[i, ...]).
In `@setup.py`:
- Around line 7-8: Remove the unused Requirement import from setup.py, leaving
install_requires to consume the raw requirements.txt strings without requiring
packaging during isolated builds.
---
Outside diff comments:
In `@MetricsReloaded/metrics/pairwise_measures.py`:
- Around line 912-928: Update n_intersection() to reduce its inputs over
self.axis, matching the axis-aware behavior of n_pos_ref() and n_union().
Preserve its existing intersection calculation while returning per-sample values
so intersection_over_union() and the related ratio no longer mix scalar
numerators with vector denominators.
- Around line 555-576: Update the empty-case guards in specificity, recall,
fbeta, negative_predictive_values, and dsc to support scalar and batched
reductions when axis excludes a batch dimension. Replace scalar if checks on
array-valued counts or denominators with element-wise masking via np.where, and
emit the existing warning when np.any of the invalid-case mask is true while
preserving nan results for invalid elements.
---
Nitpick comments:
In `@MetricsReloaded/metrics/pairwise_measures.py`:
- Around line 128-133: Update the ref_numb reduction in the pairwise measure
calculation to derive its axis from one_hot_ref.shape rather than
one_hot_pred.shape, while leaving pred_numb and the subsequent matrix
multiplication unchanged.
- Around line 96-98: Update the axis sentinel checks in the relevant
initialization logic and the code at line 359 from equality comparison with None
to identity comparison using is None, preserving the existing default-axis
behavior.
In `@MetricsReloaded/utility/utils.py`:
- Around line 282-290: Update the surrounding skeletonization logic to avoid
inferring leading-dimension count solely from axes[0]: validate that axes
represent the expected contiguous trailing spatial axes or derive the count
explicitly, and reject unsupported layouts such as (0, 2). Normalize or validate
img as binary and ensure the skeleton output uses an appropriate
boolean-compatible dtype. In the same block, split the axes None assignment onto
its own line to resolve Ruff E701.
- Around line 315-317: Update the center-of-mass helper’s empty-image branch to
return a coordinate-shaped sentinel consistent with ndimage.center_of_mass, such
as NaN-filled coordinates, instead of integer -1. Adjust com_dist only as needed
to recognize that sentinel while preserving the existing empty-image behavior
and tests.
In `@setup.py`:
- Line 24: Remove the print(requirements) debug statement from setup.py, leaving
dependency handling and the rest of the setup flow unchanged.
- Around line 53-55: Update the CI workflow’s Python test matrix to run the
versions advertised by the classifiers in setup.py, especially Python 3.11–3.13,
or remove any unsupported classifiers until compatibility is verified. Preserve
existing supported-version coverage and ensure the published Python version
metadata matches what CI validates.
In `@test/test_metrics/test_pairwise_measures.py`:
- Around line 387-400: Expand test_distance_empty to cover batched inputs by
constructing PM with a leading batch dimension and axis=(1, 2), and add coverage
for a positive smooth_dr value while preserving the expected empty-distance
results. Wrap the all-empty measured_hausdorff_distance_perc() assertion with
pytest.warns for NumPy’s RuntimeWarning, then assert the NaN result so the
warning is intentional under -W error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 83bfffc6-f1e1-47b7-b84c-0b99f42d8052
📒 Files selected for processing (4)
MetricsReloaded/metrics/pairwise_measures.pyMetricsReloaded/utility/utils.pysetup.pytest/test_metrics/test_pairwise_measures.py
| if len(cm.shape) == 3: | ||
| # Has batch dimension | ||
| weights = np.tile(weights,(cm.shape[0], 1, 1)) | ||
| numerator = np.sum(weights * cm, axis=self.axis) | ||
| denominator = np.sum(weights * exp, axis=self.axis) | ||
| # print(numerator, denominator, cm, exp) | ||
| return 1 - numerator / denominator |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Batch branch tiles weights but still reduces over self.axis, which defaults to (0, 1). For a batched confusion matrix of shape (B, K, K) the correct reduction is the last two axes; with the default axis the batch and row axes are summed instead, collapsing all batches into one scalar. Same concern applies to expected_cost (lines 156/167). Consider deriving the reduction axes from cm.ndim ((-2, -1)) rather than the constructor-level axis.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@MetricsReloaded/metrics/pairwise_measures.py` around lines 249 - 255, Update
the batched reduction logic in the affected pairwise metric method and
expected_cost to sum only the confusion-matrix axes, using the final two
dimensions for 3D inputs while preserving the configured axis for unbatched
inputs. Ensure batched results retain one value per batch instead of collapsing
the batch dimension.
| # print(prior_background, prior_foreground, alpha) | ||
| r_fp = self.fp() / self.n_neg_ref() | ||
| r_fn = self.fn() / self.n_pos_ref() | ||
| print(r_fn, r_fp) | ||
| if alpha >= 1: | ||
| ecn = alpha * r_fp + r_fn | ||
| else: | ||
| ecn = r_fp + 1 / alpha * r_fn | ||
| # print(r_fn, r_fp) | ||
| msk = alpha >= 1 | ||
| ecn = np.zeros_like(alpha) | ||
| ecn[msk] = alpha[msk] * r_fp[msk] + r_fn[msk] | ||
| ecn[~msk] = r_fp[~msk] + 1 / alpha[~msk] * r_fn[~msk] | ||
| return ecn |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Boolean-mask indexing breaks for the default (fully reduced) case. With self.axis covering all dims, alpha, r_fp, r_fn are NumPy scalars, so alpha[msk] raises TypeError: 'numpy.float32' object is not subscriptable. Use np.where, which works for both scalar and batched shapes.
🐛 Proposed fix
- r_fp = self.fp() / self.n_neg_ref()
- r_fn = self.fn() / self.n_pos_ref()
- msk = alpha >= 1
- ecn = np.zeros_like(alpha)
- ecn[msk] = alpha[msk] * r_fp[msk] + r_fn[msk]
- ecn[~msk] = r_fp[~msk] + 1 / alpha[~msk] * r_fn[~msk]
+ r_fp = self.fp() / self.n_neg_ref()
+ r_fn = self.fn() / self.n_pos_ref()
+ ecn = np.where(
+ alpha >= 1,
+ alpha * r_fp + r_fn,
+ r_fp + r_fn / alpha,
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # print(prior_background, prior_foreground, alpha) | |
| r_fp = self.fp() / self.n_neg_ref() | |
| r_fn = self.fn() / self.n_pos_ref() | |
| print(r_fn, r_fp) | |
| if alpha >= 1: | |
| ecn = alpha * r_fp + r_fn | |
| else: | |
| ecn = r_fp + 1 / alpha * r_fn | |
| # print(r_fn, r_fp) | |
| msk = alpha >= 1 | |
| ecn = np.zeros_like(alpha) | |
| ecn[msk] = alpha[msk] * r_fp[msk] + r_fn[msk] | |
| ecn[~msk] = r_fp[~msk] + 1 / alpha[~msk] * r_fn[~msk] | |
| return ecn | |
| # print(prior_background, prior_foreground, alpha) | |
| r_fp = self.fp() / self.n_neg_ref() | |
| r_fn = self.fn() / self.n_pos_ref() | |
| # print(r_fn, r_fp) | |
| ecn = np.where( | |
| alpha >= 1, | |
| alpha * r_fp + r_fn, | |
| r_fp + r_fn / alpha, | |
| ) | |
| return ecn |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@MetricsReloaded/metrics/pairwise_measures.py` around lines 640 - 648, Update
the expected calibration calculation around the ecn assignments to replace
boolean-mask indexing with np.where-based selection, preserving the alpha >= 1
and alpha < 1 formulas while supporting both NumPy scalar values from fully
reduced axes and batched arrays.
| def dice_score(self): | ||
| """ | ||
| This function returns the dice score coefficient between a reference | ||
| and prediction images | ||
|
|
||
| :return: dice score | ||
| """ | ||
| if not "fbeta" in self.dict_args.keys(): | ||
| self.dict_args["fbeta"] = 1 | ||
| elif self.dict_args["fbeta"] != 1: | ||
| warnings.warn("Modifying fbeta option to get dice score") | ||
| self.dict_args["fbeta"] = 1 | ||
| else: | ||
| print("Already correct value for fbeta option") | ||
| return self.fbeta() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
dice_score sets the wrong option key and mutates shared state. fbeta() reads dict_args["beta"] (line 811), not "fbeta", so this block never affects the result. It also mutates self.dict_args, which defaults to the shared mutable {} from __init__, leaking fbeta into every other instance created with the default; and the else branch prints debug output.
🐛 Proposed fix
- if not "fbeta" in self.dict_args.keys():
- self.dict_args["fbeta"] = 1
- elif self.dict_args["fbeta"] != 1:
- warnings.warn("Modifying fbeta option to get dice score")
- self.dict_args["fbeta"] = 1
- else:
- print("Already correct value for fbeta option")
- return self.fbeta()
+ if self.dict_args.get("beta", 1) != 1:
+ warnings.warn("Ignoring beta option to compute dice score", stacklevel=2)
+ return BinaryPairwiseMeasures(
+ self.pred,
+ self.ref,
+ connectivity_type=self.connectivity,
+ pixdim=self.pixdim,
+ dict_args={**self.dict_args, "beta": 1},
+ axis=self.axis,
+ smooth_dr=self.smooth_dr,
+ ).fbeta()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def dice_score(self): | |
| """ | |
| This function returns the dice score coefficient between a reference | |
| and prediction images | |
| :return: dice score | |
| """ | |
| if not "fbeta" in self.dict_args.keys(): | |
| self.dict_args["fbeta"] = 1 | |
| elif self.dict_args["fbeta"] != 1: | |
| warnings.warn("Modifying fbeta option to get dice score") | |
| self.dict_args["fbeta"] = 1 | |
| else: | |
| print("Already correct value for fbeta option") | |
| return self.fbeta() | |
| def dice_score(self): | |
| """ | |
| This function returns the dice score coefficient between a reference | |
| and prediction images | |
| :return: dice score | |
| """ | |
| if self.dict_args.get("beta", 1) != 1: | |
| warnings.warn("Ignoring beta option to compute dice score", stacklevel=2) | |
| return BinaryPairwiseMeasures( | |
| self.pred, | |
| self.ref, | |
| self.connectivity, | |
| pixdim=self.pixdim, | |
| dict_args={**self.dict_args, "beta": 1}, | |
| axis=self.axis, | |
| smooth_dr=self.smooth_dr, | |
| ).fbeta() |
🧰 Tools
🪛 Ruff (0.16.0)
[error] 890-890: Test for membership should be not in
Convert to not in
(E713)
[warning] 893-893: No explicit stacklevel keyword argument found
Set stacklevel=2
(B028)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@MetricsReloaded/metrics/pairwise_measures.py` around lines 883 - 897, Update
dice_score to set or validate the "beta" option consumed by fbeta(), not
"fbeta", without mutating shared default configuration state; ensure each
instance owns its own dict_args before applying the dice-specific value. Remove
the debug print from the already-correct branch while preserving the warning
when overriding a non-1 beta.
| def fppi(self): | ||
| """ | ||
| This function returns the average number of false positives per | ||
| image, assuming that the cases are collated on the last axis of the array | ||
| This function returns the average number of false positives per image | ||
| """ | ||
| sum_per_image = np.sum( | ||
| np.reshape(self.__fp_map(), -1, self.ref.shape[-1]), axis=0 | ||
| ) | ||
| return np.mean(sum_per_image) | ||
| return self.__fp_map().mean(axis=self.axis) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
fppi now returns the FP rate, not false positives per image. mean(axis=self.axis) over all axes averages the binary FP map, so a 14×14 map with 4 FPs yields ~0.02 instead of 4. Sum over the spatial axes first, then average over the image/batch axis.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@MetricsReloaded/metrics/pairwise_measures.py` around lines 899 - 903, Update
fppi to sum false positives across the spatial axes before averaging across the
image or batch axis, so it returns false positives per image rather than the
overall binary-map rate. Preserve the existing __fp_map() source and axis
configuration while separating spatial reduction from image-level averaging.
| ref_border_dist, pred_border_dist, _, _ = \ | ||
| self.border_distance() | ||
| msk = self.ref + self.pred > 0 | ||
| ref_border_dist[~msk] = np.nan | ||
| pred_border_dist[~msk] = np.nan | ||
| hdp = np.maximum( | ||
| np.nanpercentile(ref_border_dist, q=perc, axis=self.axis), | ||
| np.nanpercentile(pred_border_dist, q=perc, axis=self.axis), | ||
| ) | ||
| return hdp |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
In-place NaN masking poisons the cached border_distance() arrays. border_distance is decorated with @CacheFunctionOutput, so these writes persist; any later measured_average_distance/measured_masd/measured_hausdorff_distance call on the same instance then reduces NaN-filled arrays. Copy before masking.
🐛 Proposed fix
- ref_border_dist, pred_border_dist, _, _ = \
- self.border_distance()
- msk = self.ref + self.pred > 0
- ref_border_dist[~msk] = np.nan
- pred_border_dist[~msk] = np.nan
+ ref_border_dist, pred_border_dist, _, _ = self.border_distance()
+ ref_border_dist = np.asarray(ref_border_dist, dtype=float).copy()
+ pred_border_dist = np.asarray(pred_border_dist, dtype=float).copy()
+ msk = self.ref + self.pred > 0
+ ref_border_dist[~msk] = np.nan
+ pred_border_dist[~msk] = np.nan📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ref_border_dist, pred_border_dist, _, _ = \ | |
| self.border_distance() | |
| msk = self.ref + self.pred > 0 | |
| ref_border_dist[~msk] = np.nan | |
| pred_border_dist[~msk] = np.nan | |
| hdp = np.maximum( | |
| np.nanpercentile(ref_border_dist, q=perc, axis=self.axis), | |
| np.nanpercentile(pred_border_dist, q=perc, axis=self.axis), | |
| ) | |
| return hdp | |
| ref_border_dist, pred_border_dist, _, _ = self.border_distance() | |
| ref_border_dist = np.asarray(ref_border_dist, dtype=float).copy() | |
| pred_border_dist = np.asarray(pred_border_dist, dtype=float).copy() | |
| msk = self.ref + self.pred > 0 | |
| ref_border_dist[~msk] = np.nan | |
| pred_border_dist[~msk] = np.nan | |
| hdp = np.maximum( | |
| np.nanpercentile(ref_border_dist, q=perc, axis=self.axis), | |
| np.nanpercentile(pred_border_dist, q=perc, axis=self.axis), | |
| ) | |
| return hdp |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@MetricsReloaded/metrics/pairwise_measures.py` around lines 1174 - 1183,
Update the Hausdorff-distance calculation around border_distance() to copy
ref_border_dist and pred_border_dist before applying the ~msk NaN mask,
preventing mutation of the cached arrays while preserving the existing
percentile reduction and return behavior.
| def to_string_count(self, fmt="{:.4f}"): | ||
| result_str = "" | ||
| for key in self.measures_count: | ||
| if len(self.counting_dict[key]) == 2: | ||
| result = self.counting_dict[key][0]() | ||
| else: | ||
| result = self.counting_dict[key][0](self.counting_dict[key][2]) | ||
| result_str += ( | ||
| ",".join(fmt.format(x) for x in result) | ||
| if isinstance(result, tuple) | ||
| else fmt.format(result) | ||
| ) | ||
| result_str += "," | ||
| return result_str[:-1] # trim the last comma | ||
|
|
||
| def to_string_dist(self, fmt="{:.4f}"): | ||
| result_str = "" | ||
|
|
||
| for key in self.measures_dist: | ||
| if len(self.distance_dict[key]) == 2: | ||
| result = self.distance_dict[key][0]() | ||
| else: | ||
| result = self.distance_dict[key][0](self.distance_dict[key][2]) | ||
| result_str += ( | ||
| ",".join(fmt.format(x) for x in result) | ||
| if isinstance(result, tuple) | ||
| else fmt.format(result) | ||
| ) | ||
| result_str += "," | ||
| return result_str[:-1] # trim the last comma | ||
|
|
||
| def to_string_mt(self, fmt="{:.4f}"): | ||
| result_str = "" | ||
|
|
||
| for key in self.measures_mthresh: | ||
| if len(self.multi_thresholds_dict[key]) == 2: | ||
| result = self.multi_thresholds_dict[key][0]() | ||
| else: | ||
| result = self.multi_thresholds_dict[key][0]( | ||
| self.multi_thresholds_dict[key][2] | ||
| ) | ||
| result_str += ( | ||
| ",".join(fmt.format(x) for x in result) | ||
| if isinstance(result, tuple) | ||
| else fmt.format(result) | ||
| ) | ||
| result_str += "," | ||
| return result_str[:-1] # trim the last comma |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP '\b(measures_count|counting_dict|measures_dist|distance_dict|measures_mthresh|multi_thresholds_dict)\b' --type=py -C2Repository: Project-MONAI/MetricsReloaded
Length of output: 167
🏁 Script executed:
#!/bin/bash
set -u
echo "== repo files mentioning BinaryPairwiseMeasures =="
rg -n '\bBinaryPairwiseMeasures\b' -C 2
echo
echo "== locate pairwise_measures.py =="
fd -a 'pairwise_measures\.py$' . || true
echo
echo "== target class outline near __init__ and serializer methods =="
FILE="$(fd 'pairwise_measures\.py$' . | head -n1 || true)"
if [ -n "${FILE:-}" ]; then
ast-grep outline "$FILE" --match BinaryPairwiseMeasures --view expanded 2>/dev/null || true
echo
sed -n '1,90p' "$FILE"
echo "---- line 1150-1260 ----"
sed -n '1150,1260p' "$FILE"
fi
echo
echo "== class attribute names containing dict/measures =="
python3 - <<'PY'
from pathlib import Path
for p in Path('.').rglob('*.py'):
txt = p.read_text(errors='ignore')
if 'BinaryPairwiseMeasures' in txt:
print(f"--- {p}")
for i, line in enumerate(txt.splitlines(), 1):
if 'self.' in line and any(s in line for s in ['measure', 'dict', 'distance', 'threshold', 'count']):
print(f"{i}: {line}")
PYRepository: Project-MONAI/MetricsReloaded
Length of output: 656
Missing metrics data in these serializers. The dictionaries needed by these methods are not populated, so calling the serializers will raise errors instead of formatting metrics. Add the required metric dictionaries or remove these methods.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@MetricsReloaded/metrics/pairwise_measures.py` around lines 1196 - 1243, The
serializers to_string_count, to_string_dist, and to_string_mt reference
counting_dict, distance_dict, and multi_thresholds_dict without ensuring those
dictionaries are populated. Initialize and populate these metric dictionaries
consistently with measures_count, measures_dist, and measures_mthresh before
serialization, or remove the affected serializer methods if the data is
intentionally unsupported.
| if len(iter_ix) == 1: | ||
| # Data has channel or batch dimension | ||
| for i in img.shape[0]: | ||
| skeleton[i, ...] = skeletonize(img[i, ...]) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
for i in img.shape[0] iterates an int → TypeError. This is the single batch/channel dimension path, i.e. exactly the batched case this change enables; it fails on the first call.
🐛 Proposed fix
if len(iter_ix) == 1:
# Data has channel or batch dimension
- for i in img.shape[0]:
+ for i in range(img.shape[0]):
skeleton[i, ...] = skeletonize(img[i, ...])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if len(iter_ix) == 1: | |
| # Data has channel or batch dimension | |
| for i in img.shape[0]: | |
| skeleton[i, ...] = skeletonize(img[i, ...]) | |
| if len(iter_ix) == 1: | |
| # Data has channel or batch dimension | |
| for i in range(img.shape[0]): | |
| skeleton[i, ...] = skeletonize(img[i, ...]) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@MetricsReloaded/utility/utils.py` around lines 291 - 294, Fix the
single-dimension branch in the image skeletonization loop so it iterates over
the valid indices of the leading dimension rather than the integer value of
img.shape[0]. Preserve the existing per-slice assignment to skeleton using
skeletonize(img[i, ...]).
| #from pkg_resources import parse_requirements | ||
| from packaging.requirements import Requirement |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate setup files =="
fd -a 'setup\.py$' . | sed 's#^\./##'
echo "== setup.py contents =="
if [ -f setup.py ]; then
nl -ba setup.py | sed -n '1,140p'
fi
echo "== packaging declarations =="
for f in pyproject.toml setup.cfg setup.py requirements.txt setup.py pyproject.toml; do
if [ -f "$f" ]; then
echo "--- $f ---"
sed -n '1,220p' "$f"
fi
done
echo "== git diff summary and relevant diff =="
git diff --name-status -- setup.py pyproject.toml setup.cfg || true
git diff -- setup.py | sed -n '1,220p' || trueRepository: Project-MONAI/MetricsReloaded
Length of output: 287
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== setup.py contents =="
if [ -f setup.py ]; then
awk '{printf "%6d\t%s\n", NR, $0}' setup.py | sed -n '1,140p'
fi
echo "== project config files =="
for f in pyproject.toml setup.cfg tox.ini requirements.txt; do
if [ -f "$f" ]; then
echo "--- $f ---"
awk '{printf "%6d\t%s\n", NR, $0}' "$f" | sed -n '1,220p'
fi
done
echo "== git diff summary and relevant diff =="
git diff --name-status -- setup.py pyproject.toml setup.cfg 2>/dev/null || true
git diff -- setup.py 2>/dev/null | sed -n '1,220p' || true
echo "== imports/usages =="
python3 - <<'PY'
from pathlib import Path
p = Path("setup.py")
if p.exists():
s = p.read_text()
print("uses Requirement:", "Requirement" in s)
print("uses install_requires:", "install_requires" in s)
print("uses parse_requirements:", "parse_requirements" in s)
lines = s.splitlines()
for i, line in enumerate(lines, 1):
if "Requirement" in line or "parse_requirements" in line or "install_requires" in line or "`#from`" in line:
print(f"{i}: {line}")
PYRepository: Project-MONAI/MetricsReloaded
Length of output: 3674
Remove the unused packaging import.
setup.py passes requirements.txt raw strings to install_requires, so packaging.requirements.Requirement is not needed. Since packaging is not declared in pyproject.toml’s build-system.requires, importing it can fail in isolated/build-time environments before dependencies are installed.
Proposed fix
-#from pkg_resources import parse_requirements
-from packaging.requirements import Requirement📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #from pkg_resources import parse_requirements | |
| from packaging.requirements import Requirement | |
| `#from` pkg_resources import parse_requirements |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@setup.py` around lines 7 - 8, Remove the unused Requirement import from
setup.py, leaving install_requires to consume the raw requirements.txt strings
without requiring packaging during isolated builds.
3 participants
This PR allows for use of the metrics in
pairwise_measures.pyto be used from MONAI, in particular it enables the use of a batch dimension and a smoothing parameter. Some small modifications have also been made in some places to make the code slightly more efficient (e.g, one-hot encode only once inMultiClassPairwiseMeasures.weighted_cohens_kappa, repeated computations removed fromMultiClassPairwiseMeasures.normalised_expected_cost., etc). All unit tests pass, and the smoothing parameters is disabled by default; that is, the output of the metrics have not changed. With this PR integrated into the package, the wrapper on the MONAI side (for the pairwise metrics) should be easy to implement :)Summary by CodeRabbit
New Features
Bug Fixes