[Track] combine-roi-optimizers: union of same-type ROIs in flex & ex search#130
Merged
Conversation
Widen AtlasROI (atlas_path/hemisphere/label), SubcorticalROI (atlas_path/ label), and SphericalROI (x/y/z/radius) to scalar|list, validated (non-empty, equal length) in __post_init__ without mutating scalar inputs. Normalize to parallel lists at point-of-use so single-region configs stay byte-identical and existing saved configs/tests still load. utils.configure_roi now builds N-region union masks: per-entry mask_path/ mask_space/mask_value (enabling cross-hemisphere and cross-atlas cortical unions) with operator sequence ['intersection']+['union']*(N-1) on SimNIBS' all-True base; multi-sphere uses nested roi_sphere_center + operators while a single sphere keeps the flat form. Focality 'everything-else' complement now sets ['difference']*N to satisfy SimNIBS' equal-length check. generate_label and builder.generate_report render label/center unions '+'-joined, never as a list repr. Verified against real SimNIBS 4.6: a 2-hemi cortical union passes apply_mask's equal-length check. Adds union + back-compat + validation tests.
ROIPickerWidget now feeds the widened Phase-1 dataclasses so the flex target can be a union of same-type ROIs, while single-region entry stays byte-identical. Cortical page: - Region label QSpinBox -> comma-separated QLineEdit (_parse_labels helper). - hemi_combo gains a "Both (lh + rh)" option. - get_roi_spec expands (labels x hemispheres) into equal-length atlas_path/ hemisphere/label lists, building the per-entry lh/rh .annot path (Both x [17,53] -> label=[17,17,53,53], hemi=[lh,rh,lh,rh]). Subcortical page: - Region label QSpinBox -> comma-separated QLineEdit (list[int] labels). Spherical page: - Add an optional multi-sphere table (X/Y/Z/Radius rows) with Add/Remove; the existing spin boxes are sphere #1, so an empty table == prior single sphere. Plumbing: - _connect_signals: label inputs valueChanged -> textChanged; sphere add/remove wired to roi_changed (via lambdas so the button 'checked' bool isn't consumed as a coordinate arg). - New _collapse helper returns a scalar for a 1-element result, keeping single- region dataclasses/JSON byte-identical; lists only appear for real unions. - get_roi_params + validate handle the comma lists and multi-sphere rows; _validate_labels rejects empty/non-int/<1 labels; confirmation dialog string updated for unions. The same widget is reused as the focality non-ROI picker (self.nonroi_picker); its get_roi_spec path uses the same collapse logic, and the backend non-ROI branches route through _as_list/_broadcast, so "Specific Region" focality still round-trips for both single and multi inputs. Verified: py_compile both files; traced get_roi_spec expansion into the real Phase-1 dataclasses + utils (correct parallel lists, subject_lh/rh mask spaces, ['intersection','union'*] operators, scalar collapse for single). Backend suite green (117 passed in the config/utils/builder set; full suite 1897 passed with only the 3 documented pre-existing TestRunFlexSearch failures). GUI import tests skip (no PyQt5 in env). black clean.
Add a 'Combine selected ROIs into one target' toggle to ex-search so multiple selected spherical ROIs can be searched as a single unioned target instead of separate runs. - ExConfig: add roi_names: list[str] | None; normalize .csv suffix in __post_init__ (scalar roi_name path unchanged). - ex.run_ex_search: resolve a list of roi_files (roi_names, falling back to [roi_name]) and hand it to the engine. - ExSearchEngine: accept str | list[str] roi_file; read one center per file into roi_centers and OR the per-center spherical masks into roi_indices (single center = N=1). roi_coords still mirrors the first center for back-compat; roi_radius is shared across centers. - ex_search_tab: combine_rois_cb checkbox (default OFF) beside the radius control; when ON with >=2 selected ROIs, run one pass over all files with output/run name via '+'.join(names). Default OFF keeps the separate-runs behavior byte-identical. Tests: config normalization, run_ex_search single vs union file resolution, and engine multi-center mask union. All ex tests pass; single-ROI path stays byte-identical.
The focality non-ROI picker (self.nonroi_picker) was never validated in run_optimization(). After the region-label input became a free-text QLineEdit, an empty/non-integer non-ROI label raised a raw ValueError from get_roi_spec() -> _parse_labels() -> AtlasROI/SubcorticalROI __post_init__ inside _build_flex_config, which has no try/except at its call sites. Add a matching nonroi_picker.validate() check (only when goal=focality and non_roi_method=specific) so the reused non-ROI picker surfaces the same friendly validation as the main ROI picker.
…afe combined-ROI coord read Addresses the non-blocking review findings: - utils.py: single-region atlas/subcortical ROIs now leave mask_operator unset (SimNIBS defaults to 'intersection'), mirroring the sphere path, via new _apply_union_operator() — keeps single-region setup byte-identical. - utils.generate_label + builder._join: de-duplicate labels/hemispheres in display names so a cortical 'Both' target reads '17+53' / 'lh+rh' instead of '17+17+53+53' / 'lh+rh+lh+rh' (backend lists unchanged). - ex_search_tab: in combined mode the per-ROI coordinate read is display-only (the backend unions all roi_names); a failed read no longer aborts the whole union run and falsely reports success.
Add an interactive, searchable, multi-select region picker (AtlasRegionFinderDialog) that generalizes the analyzer's inline picker and replaces flex's two read-only region dialogs. Stores (id, name) on each item via Qt.UserRole to avoid fragile string parsing, filters by id or name, and exposes selected_ids/selected_names/selected_values plus a merge_into_lineedit load-back helper. Add a tiny hover-only HelpIcon affordance and re-export both from tit.gui.components.
- Cortical/subcortical 'List Regions' now open AtlasRegionFinderDialog (searchable, multi-select) and append selected label ids into the region-label QLineEdit via merge_into_lineedit, replacing the read-only text dumps. - Spherical page redesigned around a single full-width spheres table (one row per sphere) with stretch columns + a narrow per-row delete button; Add/Duplicate/Remove buttons manage rows. Single row still collapses to scalar x/y/z/radius so single-sphere JSON stays byte-identical. - Added set_spheres() to repopulate the table from a saved config.
…x tabs Analyzer tab: replace the inline QDialog/QListWidget region picker in show_available_regions with AtlasRegionFinderDialog(return_field="name"). Mesh regions feed as (index, name, None); voxel regions are parsed from their "Name (ID: N)" strings into (id, name, None) so the dialog returns clean names without the ' (ID:' split hack. Selected names are merged back into region_input via merge_into_lineedit, preserving the existing comma-join + dedupe behavior and the NAMES-only config contract. Ex-search tab: add a HelpIcon next to the 'Combine selected ROIs into one target' checkbox, sharing the hoisted tooltip string, wrapped in a QHBoxLayout.
…popup - atlas_region_finder: remove double-click-to-accept; selecting a row now only updates the highlight, loading happens exclusively via the Add Selected button. - help_icon: convert HelpIcon from a hover-only QLabel to a clickable QToolButton that opens the help text in an information popup (with a configurable title). - ex_search_tab: use the clickable HelpIcon (title 'Combine ROIs') next to the Combine checkbox.
…ight
Cortical ROI picker now uses analyzer-style hemisphere-prefixed region
NAMES (e.g. "lh.precentral, rh.superiorfrontal") instead of integer
label ids, and the separate hemisphere dropdown is removed (the
hemisphere is carried in each name). List Regions now lists both
hemispheres as hemi-prefixed names via AtlasRegionFinderDialog
(return_field="name"). get_roi_spec resolves each "hemi.name" token to
its .annot label index using a {(hemi, name): index} map built from
list_annot_regions; the serialized AtlasROI contract (parallel
atlas_path/hemisphere/label lists) is unchanged. validate() parses names
and warns on unknown regions. Subcortical stays integer-ID based.
Reorder ROI Definition pages/radios to Cortical (default), Subcortical,
Spherical, and make the stacked area size to the current page (collapse
non-current pages' vertical size policy to Ignored on currentChanged) so
the compact cortical/subcortical pages no longer inherit the tall
spherical spheres-table height. Flex tab lets the ROI Definition group
hug that dynamic height (Maximum vertical policy).
…tical names; docstring wording
…undled LUT Subject-space FreeSurfer atlases (e.g. aparc.DKTatlas+aseg.mgz) failed the subcortical region finder with 'LUT File Not Found' because tit only looked for a nonexistent <atlas>_labels.txt sidecar. Bundle the standard FreeSurferColorLUT.txt and, when no sidecar exists, read the atlas volume's unique nonzero labels with nibabel and map them to names/colours from the bundled table (labels present in the volume only). No FreeSurfer/mri_segstats required. MNI atlases and sidecar-bearing atlases keep their existing behavior.
Add tit/gui/components/region_chips.py providing RegionChipsWidget, a compact chips/tags widget that shows selected regions as removable, wrapping pills. Includes a standard Qt FlowLayout helper for the left-to-right wrapping arrangement. De-dups by opaque key, exposes add_item/remove/clear/set_items/keys and a changed signal, and shows a muted placeholder when empty. Exported from components/__init__.py.
…cortical ROIs Replace the comma-separated QLineEdits on the cortical and subcortical ROI pages with RegionChipsWidget. Regions are now added only via the "List Regions" finder (no free-text typing) and shown as removable, de-duplicated chips. Cortical: the finder lists both hemispheres by name and adds a chip per selection with key "hemi.name" and display "hemi.name · <annot index>". get_roi_spec/validate/get_roi_params read the chip keys and resolve each to (hemisphere, index) via the atlas .annot map, preserving the AtlasROI parallel-list contract and single-region behavior. Subcortical: the finder uses Agent A's LUT/volume resolver to list present labels by name and adds a chip per selection with key "<id>" and display "<name> · <id>". get_roi_spec reads integer ids from the chip keys and builds SubcorticalROI as before. Add set_cortical_regions()/set_subcortical_regions() to restore chips from a loaded config (resolving display names/indices where possible, else showing the bare token) plus a _subcortical_id_name_map() helper. The picker is reused as the focality nonroi_picker, so both pages round-trip through chips. Remove the dead line-edit paths and the now-unused merge_into_lineedit import.
Give the analyzer tab the same region-chips UX as the optimizer's ROI picker: each selected region is a removable 'name · #' pill instead of a comma-separated QLineEdit. Behavior-preserving — _get_regions() now returns the chip keys (the same region names the analyzer config consumes via config[region]/[regions]). - List Regions now feeds the finder real atlas label ids: cortical uses MeshAtlasManager.list_annot_regions() per hemisphere ((index, lh/rh.name)); voxel parses the real 'Name (ID: N)' label id. Chips de-dup by region name. - Move every region_input reader (validation, detail strings, output name, enable/disable, disable_controls list) onto the chips widget / _get_regions. - Drop the QLineEdit maximum-width clamp so chips reflow via height-for-width.
…lve subcortical names via bundled LUT (fixes '1: 245555.0 <name>')
Group the Analysis Configuration box into Space, Type, and two adaptive target groups (Cortical target, Spherical target). Type moves up directly after Space and drives which group is shown: cortical widgets (atlas row + List Regions, full-width region chips, tissue) vs spherical widgets (coordinate space, coordinates). Visibility is toggled from update_atlas_visibility so it stays consolidated with the existing type/space handlers and initial setup call. No widget behavior, config building, or signal wiring changed.
Move the Tissue row out of cortical_group and into the always-visible analysis_params_layout (right after the Type row). Previously the row lived inside cortical_group, which update_atlas_visibility hides in spherical mode, making the Tissue control unreachable in Voxel+Spherical even though tissue is still serialized into the config and applied by the analyzer (field-file GM/WM selection and voxel tissue masking). Enable/disable is unchanged (tissue_combo.setEnabled(not is_mesh)), so it now shows for both Voxel+Cortical and Voxel+Spherical and stays disabled in Mesh mode.
Coerce preselected to strings instead of ints and match items by str(region_id) OR name, so cortical callers keying chips by 'hemi.name' no longer crash and are correctly pre-highlighted. Int ids still match via str() coercion (back-compat).
- Clear cortical/subcortical chips when the user switches atlas or volume atlas space (user-only activated/buttonClicked signals, so programmatic refresh does not wipe a restored selection) - Add a Clear button next to List Regions on both the cortical and subcortical pages - Pre-highlight already-selected regions when reopening the region finder - Reorder the subcortical page: Atlas Space, Tissue, Volume Atlas, Region(s)
- Add a 'Clear' button next to 'List Regions' that drops all region chips. - Pre-highlight already-selected regions when opening the region finder (pass preselected chip keys to AtlasRegionFinderDialog). - Reset region chips on user-initiated Space switch or atlas change, since mesh/voxel regions and different atlases are not interchangeable. Uses space_mesh.toggled (post-setup) and the combos' activated signal so programmatic repopulation never clears the selection.
…re-selecting the same value)
…pace switch instead of clearing)
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
1 participant
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.
Goal
Let the flex-search and exhaustive-search optimizers target a union of two or more same-type ROIs as one optimization target — e.g. "maximize TI_max in both left and right hippocampus." Mirrors the analyzer's combined-ROI feature. Supported per type (no type mixing): multiple cortical atlas regions (including across both hemispheres), multiple subcortical atlas regions, multiple spherical ROIs.
How it works
SimNIBS already unions regions natively —
RegionOfInterest.apply_maskfolds per-entrymask_path/mask_space/mask_value/mask_operatorlists (androi_sphere_*lists). The toolbox previously passed one-element lists; this PR passes N parallel entries. Load-bearing detail: SimNIBS seeds the mask all-True, so a union uses['intersection'] + ['union']*(N-1)(single region leaves the operator unset → byte-identical to before). The focality complement uses['difference']*N.Cross-hemisphere cortical:
AtlasROInow carries a per-region hemisphere (each entry gets its ownlh/rh.annotpath +subject_{hemi}space), and the picker's hemisphere selector gains Both.get_roi_specexpands (hemis × labels) into equal-length lists.Phases (per
tracks/active/combine-roi-optimizers.md)config.pyROI dataclasses widened to scalar|list (normalized in__post_init__);flex/utils.pyemits union mask/sphere lists + operator sequence + focality complement;builder.pyreport formatting.roi_picker.py: comma-separated integer labels (cortical/subcortical) + Both hemisphere + multi-sphere add/remove list; the reused focality non-ROI picker handled and validated.ExConfigcarriesroi_names;engine.pyORs per-sphere masks;ex_search_tab.pyadds a Combine selected ROIs checkbox (default off → separate-runs behavior byte-identical).Review & quality
Built via an automated 3-lens adversarial review → revise loop (correctness/back-compat, SimNIBS operator sequencing, GUI↔JSON round-trip):
81f13262.cbc767be): single-region operator kept byte-identical (mirrors sphere path); display de-dup for the "Both" expansion (17+53not17+17+53+53); combined-mode coord read made non-fatal (a display-only read failure no longer aborts the union run / falsely reports success).test_opt_flex::TestRunFlexSearch(PathManager/flex env, unrelated). New tests intest_opt_flex_utils.py,test_opt_builder.py,test_opt_ex.py.black-clean on all touched files. Single-region flex + ex configs stay byte-identical;montage_visualizer/analyzer untouched.Known v1 limitation
An independent per-hemisphere cortical label set (e.g. lh.17 + rh.53 only) isn't expressible through the Both/comma UI — the backend model already supports it, so a future per-row cortical picker can add it without backend changes.
Scope
7 source + 3 test files (
tit/opt/config.py,tit/opt/flex/{utils,builder}.py,tit/opt/ex/{engine,ex}.py,tit/gui/{components/roi_picker,flex_search_tab,ex_search_tab}.py).