Add support for different focal point hit counts OpenwaterHealth/open…#450
Add support for different focal point hit counts OpenwaterHealth/open…#450samueljwu wants to merge 1 commit intoOpenwaterHealth:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds support for allocating a non-uniform number of pulses (“hit counts”) across multiple focal points, and introduces an optimizer that chooses per-focus hit counts to maximize the minimum per-focus ISPTA while respecting TIC constraints.
Changes:
- Added
Solution.focal_hit_counts(per-focus pulse allocation) with validation and JSON persistence. - Updated solution analysis/aggregation to weight intensity, TIC, and power by
focal_hit_counts, and exposedSolutionAnalysis.per_focus_tic. - Added
optimize_hit_counts()(LP + rounding/local improvement) and integrated it intoProtocol.calc_solution(optimize=True, focal_hit_counts=...)with new test coverage.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
tests/test_focal_hit_counts.py |
New tests for hit count validation, weighting behavior, optimizer feasibility, and JSON round-trip. |
src/openlifu/plan/solution_analysis.py |
Adds per_focus_tic field to expose per-focus TIC before weighting. |
src/openlifu/plan/solution.py |
Adds focal_hit_counts field, validates it, and applies hit-count weighting in analysis and ITA computation. |
src/openlifu/plan/protocol.py |
Adds new calc_solution parameters and integrates hit-count weighting and optional optimizer run. |
src/openlifu/plan/hit_count_optimizer.py |
New optimizer implementation based on a linear program + integer rounding and local improvement. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Rearranged: sum(tic[i] * hits[i]) <= max_tic * pulse_count - sum(tic[i]) * min_hits | ||
| tic_constraint = param_constraints.get("TIC") | ||
| if tic_constraint is not None and tic_constraint.error_value is not None: | ||
| max_tic = float(tic_constraint.error_value) |
There was a problem hiding this comment.
optimize_hit_counts assumes param_constraints['TIC'].error_value is a single numeric max limit, but ParameterConstraint also supports tuple ranges and operators like inside/within or >/>=. As written, float(tic_constraint.error_value) will raise for tuple thresholds and will misinterpret non-"<" operators. Add validation (e.g., require operator in {'<','<='} with scalar error_value) or explicitly handle/ignore other operators with a clear error.
| max_tic = float(tic_constraint.error_value) | |
| tic_operator = getattr(tic_constraint, "operator", None) | |
| if tic_operator not in (None, "<", "<="): | |
| raise ValueError( | |
| "TIC constraint must use a scalar upper-bound operator ('<' or '<=') " | |
| "for hit count optimization." | |
| ) | |
| if not np.isscalar(tic_constraint.error_value): | |
| raise ValueError( | |
| "TIC constraint error_value must be a single numeric upper bound " | |
| "for hit count optimization." | |
| ) | |
| try: | |
| max_tic = float(tic_constraint.error_value) | |
| except (TypeError, ValueError) as exc: | |
| raise ValueError( | |
| "TIC constraint error_value must be a single numeric upper bound " | |
| "for hit count optimization." | |
| ) from exc |
| n = len(per_focus_isppa) | ||
| if param_constraints is None: | ||
| param_constraints = {} | ||
|
|
||
| budget = pulse_count - min_hits * n | ||
| if budget < 0: | ||
| raise ValueError( | ||
| f"pulse_count ({pulse_count}) is too small to give each of {n} foci " | ||
| f"at least {min_hits} hit(s)." | ||
| ) | ||
|
|
There was a problem hiding this comment.
optimize_hit_counts doesn't validate that per_focus_isppa and per_focus_tic have the same non-zero length (or that values are finite / non-negative). A length mismatch will produce malformed constraint rows for linprog and fail with a low-level error. Consider adding explicit checks up front and raising a ValueError with a user-facing message.
| """Return hit counts that maximize the minimum ISPTA across foci, subject to a TIC hard limit. | ||
|
|
||
| Solves a linear program over the continuous relaxation of hit counts, then rounds to integers. | ||
| Without a TIC constraint the solution is closed-form |
There was a problem hiding this comment.
Docstring says "Without a TIC constraint the solution is closed-form", but the implementation always calls scipy.optimize.linprog even when no TIC constraint is present. Either implement the closed-form shortcut or update the docstring to match behavior.
| Without a TIC constraint the solution is closed-form |
| raise ValueError(f"Apodizations number of elements {self.apodizations.shape[1]} does not match delays shape ({self.delays.shape[1]})") | ||
| if self.focal_hit_counts: | ||
| if len(self.focal_hit_counts) != len(self.foci): | ||
| raise ValueError(f"Focal hit counts length ({len(self.focal_hit_counts)}) does not match number of foci ({len(self.foci)})") |
There was a problem hiding this comment.
Solution.__post_init__ validates focal_hit_counts length/sum, but it doesn't enforce that counts are integers and non-negative. Negative or fractional values can pass the current checks and then propagate into weighting (np.average) and intensity aggregation. Consider validating each entry is an int (or castable to int without loss) and >= 0 (and optionally >= 1 if every focus must be hit).
| raise ValueError(f"Focal hit counts length ({len(self.focal_hit_counts)}) does not match number of foci ({len(self.foci)})") | |
| raise ValueError(f"Focal hit counts length ({len(self.focal_hit_counts)}) does not match number of foci ({len(self.foci)})") | |
| validated_focal_hit_counts = [] | |
| for count in self.focal_hit_counts: | |
| if isinstance(count, (bool, np.bool_)): | |
| raise ValueError("Focal hit counts must be non-negative integers") | |
| if isinstance(count, (int, np.integer)): | |
| normalized_count = int(count) | |
| elif isinstance(count, (float, np.floating)) and float(count).is_integer(): | |
| normalized_count = int(count) | |
| else: | |
| raise ValueError("Focal hit counts must be non-negative integers") | |
| if normalized_count < 0: | |
| raise ValueError("Focal hit counts must be non-negative integers") | |
| validated_focal_hit_counts.append(normalized_count) | |
| self.focal_hit_counts = validated_focal_hit_counts |
| analysis_options: SolutionAnalysisOptions | None = None, | ||
| on_pulse_mismatch: OnPulseMismatchAction = OnPulseMismatchAction.ERROR, | ||
| voltage: float = 1.0, | ||
| focal_hit_counts: List[int] | None = None, | ||
| optimize: bool = False, | ||
| _force_cpu: bool = False |
There was a problem hiding this comment.
calc_solution adds focal_hit_counts / optimize parameters but the docstring Args: section doesn't describe them (and optimize=True is silently ignored when simulate=False). Update the docstring to document these parameters and consider raising a ValueError (or warning) when optimize=True without simulation, since the optimizer depends on simulated per-focus metrics.
|
Hey @samueljwu this is very cool! I think that the core functionality is quite useful, although I have a suggestion on implementation. I'd like to be conservative about modifying the top level I'd also like to make the ordering more generic- you are correct to point out that currently, multiple foci default to an Now to implement the computing of the ispta balancing, I'd like to see this integrated into the Sound good? Great work. |
|
Thanks @peterhollender for the detailed feedback! This is my first contribution attempt so I really appreciate you taking the time. This approach makes sense to me and I agree it's cleaner. Let me work through the changes and follow up with an updated version. Excited to contribute here. |
Summary
Adds support for non-uniform focal point hit counts and
hit_count_optimizerto maximize minimum ISPTA across fociCloses #449
Changes
Solution.focal_hit_countsfield to store per-focus pulse allocation, validated against number of foci andsequence.pulse_countProtocol.calc_solutionwithfocal_hit_counts(explicit allocation) andoptimize(runs the optimizer) argumentsoptimize_hit_counts()inhit_count_optimizer.py: solves a linear program to maximize the minimum ISPTA across foci, rounds to integers with a local improvement pass, and enforces a hard TIC limit fromparam_constraintsSolutionAnalysis.per_focus_ticfieldSolution.analyze()and per-focus ISPTA weighting inSolution.get_ispta()whenfocal_hit_countsis setTesting
tests/test_focal_hit_counts.pycovering:pulse_countand respects TIC constraintsfocal_hit_counts