V2 refactor - #478
Open
peterhollender wants to merge 31 commits into
Open
Conversation
| "requests", | ||
| "xarray[io]>=2026.2.0", | ||
| "numpy>=2.0.0", | ||
| "pandas>=3.0.2", |
Collaborator
There was a problem hiding this comment.
@peterhollender quick question as I am starting to look over this: do you need all these version pins? do you need this new of a pandas? this would force us to drop Python 3.10
Collaborator
|
Heads up lots of rebasing coming up |
ebrahimebrahim
self-requested a review
July 17, 2026 14:41
This was referenced Jul 17, 2026
ebrahimebrahim
force-pushed
the
v2_refactor
branch
from
July 17, 2026 15:49
38a365e to
baadec4
Compare
ebrahimebrahim
requested changes
Jul 17, 2026
ebrahimebrahim
left a comment
ebrahimebrahim
left a comment
Collaborator
There was a problem hiding this comment.
(1) The PR description should be updated and cleaned up.
- It makes outdated assertions about how many commits and files were changed -- these are not necessary. It also makes a few claims that aren't true/relevant (e.g. are distance units in
SolutionAnalysisOptionsreally restricted to mm/cm/m? not seeing that. what does it mean forsolution_idto be "auto-cleared when array_transform changes"? maybe SlicerOpenLIFU development is leaking into the notes) - It would be nice to also cut down the PR description and improve its readability for humans, to better understand the changes.
We can re-use the text for release notes if it is sufficiently well edited, so it is worth cleaning up!
(2) Many version constraints have been added -- do these all have a good justification? Since openlifu-python is a utility library it should be as flexible as it can reasonably be when it comes to version constraints, so that people can use it in a variety of environments with other things in them.
The version constraints xarray>=2026.2.0, pandas>=3.0.2, and test-extra scikit-image>=0.26.0 in particular kick out Python 3.10! We need to either chill with the constraints or stop declaring Python 3.10 support.
| simulation_result = self.simulation_result | ||
| simulation_result_scaled = rescale_coords(simulation_result, options.distance_units) | ||
| masks = [] | ||
| wavelength = options.ref_sound_speed / self.pulse.frequency |
Collaborator
There was a problem hiding this comment.
This wavelength is in meters -- should it be converted to options.distance_units as well?
| ) | ||
| else: | ||
| raise ValueError("Invalid 'on_conflict' option. Use 'error', 'overwrite', or 'skip'.") | ||
| analysis_filepath.parent.mkdir(parents=True, exist_ok=True) |
Collaborator
There was a problem hiding this comment.
There are two problems that arise from how solution analyses are being written and linked back to solutions:
- staleness: You could write a solution, then write a solution analysis, and then make a change to the solution (keeping the same ID). The solution analysis remains associated to the solution but it is no longer an analysis of that solution. This could lead to danger
- orphan analysis: A solution analysis can be written out for a solution that does not exist. E.g. this succeeds:
db.write_solution_analysis(session, "mistyped-id", analysis).
I think we can do the following to address these, respectively:
- When a solution is being over-written, delete or invalidate any existing analysis
- When writing an analysis, fail if the solution is not indexed or the soluton file does not exist
Collaborator
There was a problem hiding this comment.
Is this file meant to be included in this PR? If so maybe it belongs elsewhere like examples/tools
Comment on lines
+649
to
+655
| if interface is None: | ||
| if _SDKLIFUInterface is None: | ||
| raise ImportError( | ||
| "openlifu_sdk is required to auto-create a LIFUInterface; " | ||
| "install it or pass an explicit `interface=` argument." | ||
| ) | ||
| interface = _SDKLIFUInterface() |
Collaborator
There was a problem hiding this comment.
In the event that we reach this line interface = _SDKLIFUInterface() the created interface needs to be closed to release the PID lock later before this function returns (in a finally block or something like that). Otherwise a call to get_connected() could result in that lock being held up (i.e. this is a resource leak)
This was
linked to
issues
Jul 17, 2026
This was
linked to
issues
Jul 17, 2026
…terHealth/SlicerOpenLIFU#611) A Solution on disk is a compact, hardware-ready object with no intrinsic knowledge of which target, transducer, protocol, or transducer pose produced it. Add a lightweight per-solution provenance record at the Session level so consumers can (a) link a loaded Solution back to its context and (b) cascade-delete solutions when the target / virtual-fit / transducer-tracking result they were computed against is removed. New SolutionInfo dataclass: - solution_id, protocol_id, target_id, transducer_id - transducer_transform_source: str restricted to 'virtual_fit' or 'localization' via __post_init__ (VALID_TRANSDUCER_TRANSFORM_SOURCES ClassVar). Additional identifying fields (e.g. VF rank, TT result id) can be added later so consumers can pin down exactly which transform was used. Session gets a new solutions: List[SolutionInfo] field. from_dict reconstructs each entry via SolutionInfo(**s) and defaults to [] when the field is absent (legacy sessions). to_dict serializes via asdict(). No changes to the on-disk Solution format. Semantics for downstream consumers (implemented in SlicerOpenLIFU): every Solution belonging to a Session should have a matching SolutionInfo entry. Solutions on disk with no matching entry are considered orphaned -- they are not loaded and are purged from the session's solutions/ directory on save. Tests: added round-trip test for Session.solutions through write_session/load_session, plus a validation test asserting SolutionInfo rejects unknown transducer_transform_source values.
peterhollender
force-pushed
the
v2_refactor
branch
from
July 23, 2026 18:25
f3946a9 to
02f7785
Compare
…ealth/SlicerOpenLIFU#611) delete_solution mirrors delete_session: rmtree the solution directory and trim the solutions.json index, with ERROR/SKIP on_conflict handling. purge_orphaned_solutions reconciles the on-disk solution set against the authoritative session.solutions list of SolutionInfo entries, deleting any solution directory whose id is not tracked. Used by SlicerOpenLIFU to cascade-purge on session save. An empty session.solutions list is authoritative and purges every on-disk solution.
…SlicerOpenLIFU#611) approved: session-provenance approval axis, distinct from any Solution-level approval. Defaults to False. Used by the upcoming Solutions Table UI. computed_at: Optional[datetime] capturing when the Solution was first computed. Serialized via PYFUSEncoder as ISO 8601. Defaults to None for legacy entries that predate the field. __post_init__ transparently parses ISO strings so Session.from_dict does not need special handling.
…roved) (OpenwaterHealth/SlicerOpenLIFU#611) Approval is a user-session-time decision -- Python-side Solution construction and Protocol.calc_solution have no approval concept, and no code ever called anything like solution.approve(). Move it off the openlifu.plan.Solution dataclass entirely and let SolutionInfo.approved on the Session be the single source of truth. Solution.from_dict tolerates and silently drops any legacy approved key so existing on-disk Solution JSONs continue to load. The cloud SolutionDto is left alone since it mirrors a separate cloud-API contract.
Add an optional ``array_transform: ArrayTransform | None`` field to ``SolutionInfo`` capturing the transducer array-to-volume transform matrix used when the corresponding ``Solution`` was computed. Defaults to ``None`` for legacy sessions saved before the field existed. Motivated by SlicerOpenLIFU#622: rendering a computed Solution's peak-negative- pressure volume requires reproducing the exact transducer pose used at compute time, otherwise the PNP moves with whichever virtual-fit / transducer-tracking result is currently approved -- and for a pre-solution whose VF approval was later revoked, the compute-time pose is otherwise unrecoverable. Storing the matrix on the provenance record makes solution display invariant under later approval churn. ``__post_init__`` accepts a dict transparently so ``Session.from_dict`` decoding of ``solutions`` entries produces the correct ``ArrayTransform`` instance without special-casing at the call site (mirroring the existing ISO-string handling for ``computed_at``). Round-trip tests added in ``tests/test_database.py``.
Add an optional ``transducer_transform_source_id: str | None`` field to
``openlifu.db.session.SolutionInfo`` so downstream consumers can distinguish
"the specific VF or TT result that produced this solution" from the coarser
``transducer_transform_source`` ("virtual_fit" vs. "localization") kind.
Defaults to ``None`` for backward compat with sessions saved before this
field existed.
Motivated by SlicerOpenLIFU's need to badge solutions in the Solutions
table with a source-liveness status (Live / Revoked / Missing / Legacy) and
warn / block sending stale solutions to hardware. Since openlifu-python#491
gave us a stable pose (``array_transform``), what was missing was a way to
tell whether the specific VF / TT result that produced the solution is
still (a) present in the session, (b) still approved.
The field is opaque -- interpretation is source-dependent and delegated to
the downstream app. SlicerOpenLIFU will use ``"<target_id>:<rank>"`` for VF
(composite key that stays stable across approval-only changes) and the
``TransducerTrackingResult.id`` for TT.
Round-trip tests added.
Fixes #492.
Introduces `subjects/{subject_id}/solutions/{solution_id}/` as an
alternative to the legacy `subjects/{sid}/sessions/{sess}/solutions/{sid}/`
layout. This is the storage half of the session-split refactor
(SESSION_SPLIT_DESIGN.md in SlicerOpenLIFU): in the new model,
Plans and the split sessions hold `SolutionInfo` refs, and the actual
Solution files sit at subject scope so a single Solution can be referenced
by a PlanningSession (as a pre-solution), a Plan (finalized snapshot),
and/or a SonicationSession (as the final solution) without duplication.
New Database methods (subject-scoped, no Session argument):
* `get_subject_solutions_filename(subject_id)`
* `get_subject_solution_dir(subject_id, solution_id)`
* `get_subject_solution_filepath(subject_id, solution_id)`
* `get_subject_solution_analysis_filepath(subject_id, solution_id)`
* `get_subject_solution_ids(subject_id)` -- returns [] for a subject
with no solutions index (missing file is a legitimate empty state
here, unlike the noisy legacy `get_solution_ids`).
* `write_subject_solution_ids(subject_id, ids)`
* `write_solution_at_subject_scope(subject_id, solution, on_conflict=...)`
* `load_solution_at_subject_scope(subject_id, solution_id)`
* `write_solution_analysis_at_subject_scope(subject_id, sid, analysis, ...)`
* `load_solution_analysis_at_subject_scope(subject_id, sid)`
* `delete_solution_at_subject_scope(subject_id, sid, on_conflict=...)`
The legacy session-scoped methods (`write_solution`, `load_solution`,
`delete_solution`, `purge_orphaned_solutions`, `get_solution_ids`,
`get_solution_filepath`, ...) are untouched. Both scopes coexist; a
solution with the same id may exist in both scopes without collision
(covered by a regression test).
Naming uses the `_at_subject_scope` suffix to make the transitional
period unambiguous. When the session-split refactor lands and the old
`Session` code path is deleted, these will be renamed to their
unqualified forms.
Introduces the three new data types that replace the omnibus
``openlifu.db.Session`` in the split-session refactor. Legacy ``Session``
is untouched.
* **Plan** (``openlifu/db/plan.py``): immutable finalized output of a
PlanningSession. Pins target + volume + protocol + transducer +
approved virtual-fit pose (``array_transform``). Optionally carries a
list of ``SolutionInfo`` refs (``pre_solutions``) to solutions
computed at planning time; the actual Solution files live at subject
scope (``subjects/{sid}/solutions/``) and are shared with the parent
PlanningSession without duplication.
* **PlanningSession** (``openlifu/db/planning_session.py``): mutable
working document. Owns targets, ``virtual_fit_results``,
``pre_solutions`` (``SolutionInfo`` refs), and
``finalized_plan_ids``. Multiple Plans per session are supported;
every Plan a session has produced is tracked in
``finalized_plan_ids``.
* **SonicationSession** (``openlifu/db/sonication_session.py``): mutable
at-treatment-time session. References a Plan by ``plan_id`` (frozen
input; volume/target/protocol/transducer/array_transform come from the
Plan). Owns ``photoscan_ids`` (subject-scoped storage,
session-scoped ownership), ``photoscan_registrations``,
``transducer_tracking_results``, ``solution: SolutionInfo | None``
(the ONE final solution, not a list; multi-solution generation is
a future feature), and ``run_ids``.
Each class follows the same manual serialization pattern as
``Session`` / ``Protocol`` / ``Solution``: to_dict / from_dict /
to_json / from_json / to_file / from_file.
``openlifu.db.__init__`` now re-exports ``Plan``, ``PlanningSession``,
and ``SonicationSession`` alongside ``Database`` / ``Session`` /
``Subject`` / ``User``.
Tests in ``tests/test_split_sessions.py`` cover:
* Default id/name auto-population (parallels Session behavior).
* Round-trip through dict, JSON, and disk file for each type.
* SonicationSession.solution is Optional[SolutionInfo], not a list
(guards against reintroducing multi-solution scope creep).
* SonicationSession has no volume_id / target field (guards against
reintroducing fields that were explicitly moved to Plan).
* PhotoscanRegistration / TransducerTrackingResult / SolutionInfo
round-trip correctly through SonicationSession.
* SolutionInfo shared between PlanningSession.pre_solutions and
Plan.pre_solutions is the load-bearing invariant for the future
finalize_plan().
* Export smoke test.
25 new tests. Full test_split_sessions.py + test_database.py passes
(106/106). ``test_version`` failure is a pre-existing stale-install
issue unrelated to this change.
Fills out the openlifu-python side of the split-session refactor with Database CRUD for the three new types and a subject-scoped storage path for photoscans. Legacy ``write_session`` / ``load_session`` / ``delete_session`` and ``write_photoscan`` / ``load_photoscan`` are untouched. **Plan / PlanningSession / SonicationSession CRUD** Path helpers, index helpers, and a three-method write/load/delete surface (``write_plan``, ``load_plan``, ``delete_plan``, and the equivalents for PlanningSession and SonicationSession) added to ``Database``. All new methods take ``subject_id`` as a plain string rather than a ``Subject`` instance -- matches the subject-scoped solutions API added in the previous commit and sets us up cleanly for a future relational-DB migration where these become FK-normalized tables. Validation: * ``write_plan`` / ``write_planning_session`` / ``write_sonication_session`` check that the session's ``subject_id`` matches the ``subject_id`` argument, if non-None. * ``write_planning_session`` additionally validates ``virtual_fit_results`` entries reference known target ids and have at least one transform each (parallels the ``write_session`` behavior). * Deletion of a Plan / PlanningSession / SonicationSession does NOT cascade into subject-scoped Solutions or subject-scoped Photoscans. Those are independent artifacts; orphan cleanup is a separate, deliberate step. **Subject-scoped Photoscan storage** Path helpers, index helpers, and a ``_at_subject_scope``-suffixed CRUD surface for photoscans: ``write_photoscan_at_subject_scope``, ``load_photoscan_at_subject_scope``, ``get_photoscan_absolute_filepaths_info_at_subject_scope``, ``delete_photoscan_at_subject_scope``. The two storage paths are disjoint: a subject-scoped photoscan and a legacy session-scoped photoscan with the same id coexist on disk (regression test included). **Tests** 15 new tests in ``tests/test_split_sessions.py`` cover: * Plan write/load/delete + on_conflict semantics + subject_id mismatch rejection + missing-file error. * PlanningSession write/load/delete + on_conflict semantics + subject_id mismatch rejection + missing-file error + VF validation (unknown target, empty transform list). * SonicationSession write/load/delete + on_conflict semantics + subject_id mismatch rejection + missing-file error, including round-trip of nested PhotoscanRegistration / TransducerTrackingResult / SolutionInfo. * Subject-scoped photoscan write/load/delete + get_photoscan_absolute_filepaths_info_at_subject_scope + no-collision with legacy session-scoped storage. * Integration: a single Solution at subject scope is referenceable from a PlanningSession's ``pre_solutions`` AND a Plan's ``pre_solutions`` after round-trip through disk, with the underlying Solution existing exactly once. Sketches the finalize_plan() invariant (implemented in a later commit). Full test_split_sessions.py: 40/40 pass. Full test suite: 362/362 pass. Note: the pinned pre-commit ruff (v0.4.1) and current local ruff (0.15.14) disagree on PT001 fixture-parentheses direction. Following pre-commit's preference in this file.
Feedback from Slicer-side integration testing: with the previous
naming, PlanningSession IDs had to carry a ``_planning`` suffix (and
Plans a ``_plan`` suffix, SonicationSessions a ``_sonication``
suffix) to keep them visually distinguishable across the three list
views. That's noise the ID itself doesn't need to carry -- the file
type is already unambiguous from the directory the file lives in.
Move the type discriminator from the id-suffix convention to the
filename extension. Same id (e.g. ``neuromod_1x_demo``) can now be
reused across a PlanningSession, the Plans finalized from it, and
Sonication Sessions launched from those Plans; the files are still
uniquely named on disk.
Extension convention:
* PlanningSession -> ``{id}.planning.json``
* Plan -> ``{id}.plan.json``
* SonicationSession -> ``{id}.sonication.json``
* subject-scoped Solution -> ``{id}.solution.json``
* subject-scoped SolutionAnalysis -> ``{id}.solution_analysis.json``
Legacy session-scoped solution filenames (``{sid}.json`` under
``sessions/{sess_id}/solutions/``) are unchanged; those paths get
retired with the rest of the legacy ``Session`` layout in the final
row of the staging plan.
This aligns with the "sets us up cleanly for a future relational-DB
migration" rationale in SESSION_SPLIT_DESIGN.md: filename-based type
tags map more naturally to a per-type table + primary key than
id-suffix conventions do.
Tests updated to reflect the new extension in explicit-path
assertions. Round-trip tests were unaffected. Full suite: 121/121.
update ISPTA for multiple foci
peterhollender
force-pushed
the
v2_refactor
branch
from
July 31, 2026 19:27
9cdd64a to
73d4577
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR Summary —
v2_refactorintomain21 commits, 34 files, +2748/−244. Fixes/relates to issues #8, #467, #468.
High-level themes
Sessiontransducer-tracking + photoscan data into a first-classPhotoscanRegistrationtype.SolutionAnalysispersistence.OpenLIFUFieldDatagains units/precision/display fields;__repr__/_repr_html_/get_summary()added across the model.Breaking / semantic changes to call out for reviewers
openlifu.db.sessionPhotoscanRegistration:photoscan_id,transform,approval,id.TransducerTrackingResult:photoscan_to_volume_transform,transducer_to_volume_tracking_approved,photoscan_to_volume_tracking_approved.photoscan_registration_id,approval(replacestransducer_to_volume_tracking_approved),id,target_id.Sessionadds:solution_id(auto-cleared whenarray_transformchanges), explicitphotoscans/photocollectionslists, andphotoscan_registrations.Session.from_dictsynthesizesPhotoscanRegistrationobjects with id{photoscan_id}__pr__{n:02d}from old embedded PV transforms so existing sessions load.openlifu.db.databasewrite_solution_analysis,load_solution_analysis,get_solution_analysis_filepath→{solution_dir}/{solution_id}_analysis.json.load_sessionnow auto-populates missingphotoscans/photocollectionsfrom index files and drops orphaned TT results whosephotoscan_registration_idis missing (logs a warning).openlifu.plan.solution/solution_analysisSolutiongainsget_mask(...),get_mainlobe_mask,get_sidelobe_mask.Solution.analyze()computes mainlobe centroid from intensity (ipa_mainlobe, cutoffipk * 10^(−3/20)) instead of pressure. Focal-point metrics will shift slightly vsmain.SolutionAnalysisOptionsdefault changes (numeric → auto-derived):mainlobe_aspect_ratio:(1, 1, 5)→(1, 1, 7)mainlobe_radius:2.5e-3→None(derived from beamwidth)beamwidth_radius:5e-3→None(derived from2.0 * fnum * wavelength)sidelobe_radius:3e-3→None(beamwidth × 1.5)sidelobe_zmin:1e-3→None(falls back to 10 mm scaled todistance_units,DEFAULT_SIDELOBE_ZMIN_MM = 10.0)distance_unitsrestricted to("mm", "cm", "m")estimated_tx_temperature_rise_Clabel renamed: "Estimated TX Temperature Rise" → "Est. Transmitter Heating".model_tx_temperature_risebounds-check messages downgraded fromWARNING→DEBUG(they were noisy). Corresponding pytest cases removed.openlifu.cloudCloud.__init__(environment: str = ENV_PROD);Api.__init__(api_url);Websocket.__init__(api_url, update_callback).openlifu.cloud.const.API_URLremoved; replaced byAPI_URL_PRODandAPI_URL_DEV.Cloud._componentsno longer registersSystems(matches sample-DB layout).openlifu.xdc.transducerarrayTransducerArraygains device-connection & config-validation surface:TransducerArray.get_connected(...),TransducerArray.from_module_user_configs(...),TransducerArray.to_device_config(), plus a full repr suite (__repr__,__str__,_repr_pretty_,_repr_html_).openlifu.xdc:DeviceConfigMismatchError,arrays_structurally_equal._validate_device_config_against_connected,_build_meshless_default_template,_canonicalize_array_for_compare,get_gap_from_angle.openlifu.util.annotationsOpenLIFUFieldDataNamedTuple → frozen dataclass with 5 new optional fields (units,display_units,unit_options,precision,units_field). Backwards-compatible: 2-arg positional construction still works, attribute access unchanged.util/field_display.pyfor reading annotation metadata off dataclass fields.Additive-only changes (no reviewer concern)
bf/apod_methods/*,bf/delay_methods/direct.py,bf/focal_patterns/*,bf/pulse.py,bf/sequence.py,sim/sim_setup.py,seg/virtual_fit.py,seg/seg_method.py,xdc/element.py: all addget_summary(), richer repr, and expandedOpenLIFUFieldDatametadata; no signature changes on existing methods.util/units.rescale_coords: parameter now acceptsDataset | DataArray.param_constraint.py: symbol swap inPARAM_STATUS_SYMBOLS(❗ → ⚠️,❌ → ⛔).Tests
tests/test_transducer.py(+400) andtests/test_transducer_array_device_config.py(+233) — cover the newTransducerArraytype, device-config match/mismatch, and repr paths.tests/test_database.py(+55) exercisesload_solution_analysis/write_solution_analysisand the photoscan-registration index behavior.test_solution_analysiscases whose warnings were downgraded to debug.test_package::test_versionfailure (stale editable install in local venv; not caused by this branch).Bug caught during review
Original diff added
"run_virtual_fit"toopenlifu.plan.__init__'s__all__without an import, which would have causedAttributeErroronopenlifu.plan.run_virtual_fit. Fixed in185beee—run_virtual_fitremains accessible viaopenlifu.seg.Downstream compatibility
SlicerOpenLIFU has been co-developed against this branch and already calls the new APIs (
TransducerArray.get_connected,Database.load_solution_analysis/write_solution_analysis,PhotoscanRegistration,Cloud(env), newOpenLIFUFieldDatafields). No SlicerOpenLIFU references to any removed symbol remain. This branch is a required bump for the in-flight SlicerOpenLIFU work.