Summary
There are two implementations of get_analyses_by_coordinate in the Studyset class with different signatures and behaviors. The latter definition overrides the former, making the first implementation effectively dead code.
Details
The first implementation:
- Accepts
xyz and radius r
- Uses
scipy.spatial.distance.cdist
The second implementation:
- Accepts
xyz with either r (radius) or n (number of nearest analyses)
- Uses manual Euclidean distance computation
- Overrides the first definition entirely
This leads to:
- Dead/unreachable code
- Potential confusion for contributors and users
- Inconsistent API design
Proposed Solution
- Remove the duplicate implementation
- Keep a single unified method supporting both:
- radius-based filtering (
r)
- nearest-neighbor selection (
n)
- Add validation to ensure only one of
r or n is provided
- Update docstrings accordingly
Additional Suggestions
- Add tests for both modes (
r and n)
- Consider deprecating older behavior if needed
Why this matters
This improves:
- API consistency
- Code maintainability
- Developer experience
Happy to open a PR for this if the approach is approved.
Summary
There are two implementations of
get_analyses_by_coordinatein the Studyset class with different signatures and behaviors. The latter definition overrides the former, making the first implementation effectively dead code.Details
The first implementation:
xyzand radiusrscipy.spatial.distance.cdistThe second implementation:
xyzwith eitherr(radius) orn(number of nearest analyses)This leads to:
Proposed Solution
r)n)rornis providedAdditional Suggestions
randn)Why this matters
This improves:
Happy to open a PR for this if the approach is approved.