V2 refactor#478
Conversation
| "requests", | ||
| "xarray[io]>=2026.2.0", | ||
| "numpy>=2.0.0", | ||
| "pandas>=3.0.2", |
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
|
Heads up lots of rebasing coming up |
38a365e to
baadec4
Compare
ebrahimebrahim
left a comment
ebrahimebrahim
left a comment
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 |
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) |
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
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
| 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() |
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)
…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.
f3946a9 to
02f7785
Compare
3 participants
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.