Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements a comprehensive "Metrics plugin" system that adds support for custom metrics and preprocessing functions in the hip ultrasound analysis pipeline. The changes introduce pluggable architecture for extending the analysis capabilities.
- Adds custom metric functions and segmentation preprocessing hooks to the HipConfig
- Implements line extension algorithm for improved point positioning calculations
- Enhances drawing system with polynomial-based smooth skeleton rendering and improved error handling
Reviewed Changes
Copilot reviewed 22 out of 23 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| retuve/keyphrases/subconfig.py | Adds full_metric_functions and seg_preprocess_functions parameters to HipConfig |
| retuve/funcs.py | Implements plugin execution logic for custom metrics and preprocessing functions |
| retuve/utils.py | Extends point_left_right function with 1000-pixel line extension algorithm |
| retuve/classes/draw.py | Enhances skeleton drawing with polynomial fitting for smooth curves |
| retuve/hip_us/modes/seg.py | Updates segs_2_landmarks_us to return ilium angle baselines |
| retuve/hip_us/metrics/alpha.py | Modifies find_alpha_landmarks to calculate and return angle baseline |
| retuve/hip_us/multiframe/graf.py | Replaces global DO_CALIBRATION with config-based do_calibration |
| retuve/hip_us/classes/dev.py | Adds average_ilium_angle_baseline field to DevMetricsUS |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
retuve/classes/draw.py
Outdated
| fill=self.config.hip.midline_color.rgba(), | ||
| ) | ||
|
|
||
| if len(skel) < 2: |
There was a problem hiding this comment.
Need to try and simplify this
|
|
||
| # Merge any custom dev metrics collected earlier (e.g., from full_metric_functions) | ||
| try: | ||
| if hasattr(hip_datas, "dev_metrics_custom") and hip_datas.dev_metrics_custom: |
There was a problem hiding this comment.
We should remove these hasattr's in favor of a proper object
| from retuve.logs import log_timings | ||
|
|
||
|
|
||
| def _polyfit_replace_apex( |
There was a problem hiding this comment.
This is insanely long
| ) | ||
| ) | ||
|
|
||
| # Also aggregate any custom per-frame metric function outputs |
There was a problem hiding this comment.
Should be its own function, maybe we put all the custom stuff in its own file?
| graf_conf, | ||
| ) | ||
|
|
||
| # Run any custom post-draw hooks after base drawing (standard signature) |
| self.frame_no: int = None | ||
| self.recorded_error: RecordedError = RecordedError() | ||
|
|
||
| def get_metric(self, name: str): |
There was a problem hiding this comment.
This does not seem nessasary
There was a problem hiding this comment.
Actually, I think we have this for the Ultrasound equivalent so maybe it stays?
| overlay = draw_ihdi(final_hip, overlay, config) | ||
| overlay = draw_tonnis(final_hip, overlay, config) | ||
|
|
||
| # Run any custom post-draw hooks after base drawing (standard signature) |
There was a problem hiding this comment.
Own function, the __name__'s scare me
| hip_datas_xray = landmarks_2_metrics_xray(landmark_results, config) | ||
|
|
||
| # Run custom per-image metric functions (standardized signature) | ||
| per_frame_funcs = getattr(config.hip, "per_frame_metric_functions", []) or [] |
| results, shape = pre_process_segs_us(results, config) | ||
|
|
||
| # Run any custom segmentation pre-process hooks | ||
| for preprocess in getattr(config.hip, "seg_preprocess_functions", []) or []: |
| hip_datas.ilium_angle_baselines = ilium_angle_baselines | ||
|
|
||
| # Initialize a container for any custom dev metrics returned by full_metric_functions | ||
| if ( |
There was a problem hiding this comment.
Own function / We should remove these hasattr's in favor of a proper object
| x2, y2 = p2 | ||
| x3, y3 = p3 | ||
|
|
||
| # Extend the line segment by 1000 pixels in both directions |
There was a problem hiding this comment.
There has to be a way to make this code more consise
| metrics_2dus = {} | ||
| for _dict in metrics_2d_us["metrics"]: | ||
| metrics_2dus.update(_dict) | ||
| for item in metrics_2d_us["metrics"]: |
There was a problem hiding this comment.
Own function, maybe apply DRY when we have all this together
| dev_metrics: {} | ||
| us_sweep: | ||
| metrics: | ||
| - Metric2D(name=alpha, value=69.2) # Modified |
There was a problem hiding this comment.
This is a bug that needs fixed
| @@ -0,0 +1,88 @@ | |||
| description: Your description here. | |||
There was a problem hiding this comment.
Need to fill this in
Signed-off-by: Sharpz7 <adam.mcarthur62@gmail.com>
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
|
|
||
| # Fallback: draw values of registered per-frame metrics if present | ||
| # Only run this fallback when there are NO custom post-draw hooks to avoid duplicates | ||
| per_frame_funcs = getattr(config.hip, "per_frame_metric_functions", []) or [] |
There was a problem hiding this comment.
This is a pointless fallback
No description provided.