ENH: support multiple simultaneous overlays in Brain.add_data - #13995
Conversation
|
Yes @payamsash that sounds good to me! Related to:
We also have an issue with interactivity, in that the |
|
for the interactivity I will open another PR (but probably not soon), as I now have a good view how it works, but I think its quite challenging (at least compared to this one). |
…k from previous PR
…one and fix _get_range to use _active_data
d46dc2e to
a6a9079
Compare
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
|
FYI if you In the meantime I've started CircleCI and will review |
larsoner
left a comment
There was a problem hiding this comment.
Okay I get the architecture here now. It makes sense. But I have a slightly different proposal that will help minimize diff, and thus help preserve blame and reduce review burden. Instead of what is currently implemented:
- Modify
self._datato be the dict-of-dicts storing all datasets - Add
self._active_data_keyto track what the "active" key is - Add
self._active_dataas a@propertythat wraps toself._data[self._active_data_key]
How about
- Add
self._all_datato be the dict-of-dicts storing all datasets - Add
self._dataas a@propertythat wraps toself._all_data[self._active_data_key]
It makes the code naming slightly worse, but it should make the diff much smaller, which helps maintainability (because of how blame, mostly).
|
@payamsash I am not notified of new commits so if you need re-review feel free to ping me directly! |
|
Changes LGTM, marking for merge-when-green -- thanks in advance @payamsash ! |
* upstream/main: Make scrollbar handlers draggable (mne-tools#14040) [pre-commit.ci] pre-commit autoupdate (mne-tools#14052) Warn when Epochs events fall outside the raw data range (mne-tools#12989) (mne-tools#14004) Ensure epochs being concatenated have compatible event ids (mne-tools#14051) Widen main content area (mne-tools#14015) MAINT: remove dead gain/bits/value_range fields from _read_header in … (mne-tools#14047) ENH: replace `_get_blocks` binary reader with mffpy Reader API (mne-tools#14043) MAINT: Update dependency specifiers (mne-tools#14048) [dependabot]: Bump the actions group with 2 updates (mne-tools#14049) Simplify doc building with more refleak (mne-tools#14045) ENH: add overlay Brain GUI (mne-tools#14031) ENH: support multiple simultaneous overlays in Brain.add_data (mne-tools#13995) Add option to show a zero line in browser (mne-tools#14018) FIX: pass cmap name string not tuple to interactive topomap slider kwargs (mne-tools#14039) Doc/add ai policy pointer (mne-tools#14037) Allow subclasses of FigureClass to be passed to plot_raw/plot_epochs (mne-tools#13979) MAINT: Replace manual PNS binary block reader in `_read_segment_file` with `mffpy` (mne-tools#14030)
* upstream/main: (206 commits) Improve type checks (mne-tools#14036) Make scrollbar handlers draggable (mne-tools#14040) [pre-commit.ci] pre-commit autoupdate (mne-tools#14052) Warn when Epochs events fall outside the raw data range (mne-tools#12989) (mne-tools#14004) Ensure epochs being concatenated have compatible event ids (mne-tools#14051) Widen main content area (mne-tools#14015) MAINT: remove dead gain/bits/value_range fields from _read_header in … (mne-tools#14047) ENH: replace `_get_blocks` binary reader with mffpy Reader API (mne-tools#14043) MAINT: Update dependency specifiers (mne-tools#14048) [dependabot]: Bump the actions group with 2 updates (mne-tools#14049) Simplify doc building with more refleak (mne-tools#14045) ENH: add overlay Brain GUI (mne-tools#14031) ENH: support multiple simultaneous overlays in Brain.add_data (mne-tools#13995) Add option to show a zero line in browser (mne-tools#14018) FIX: pass cmap name string not tuple to interactive topomap slider kwargs (mne-tools#14039) Doc/add ai policy pointer (mne-tools#14037) Allow subclasses of FigureClass to be passed to plot_raw/plot_epochs (mne-tools#13979) MAINT: Replace manual PNS binary block reader in `_read_segment_file` with `mffpy` (mne-tools#14030) Fix transition bandwidth reported in 'filter too short' error (mne-tools#11406) (mne-tools#14005) ENH: Show the current time as a vertical line in plot_evoked_topo (mne-tools#14032) ...
Reference issue (if any)
Closes #11503.
What does this implement/fix?
The rendering side is already ready (#13970), so it can keep multiple named overlays together. This PR is on the
Brainside so it can track more than one dataset at a time, and connectingremove_existing=Falseto actually work. so WDYT about these steps @larsoner?self._datato index datasets by key instead of overwriting, so multiple calls toadd_data(..., key="stats")andadd_data(..., key="data")can happen.remove_existing=Falseto callLayeredMesh.add_overlaywith a new key.