Skip to content

Conversation

@hoechenberger
Copy link
Member

@hoechenberger hoechenberger commented Jan 23, 2022

Screen.Recording.2022-01-23.at.21.07.58.mov
  • CoregistrationUI gained a Save button, which will save the current fiducial selection into the standard location subjects_dir/subject/bem/subject-fiducials.fif). It is not possible to pick a custom location. (Will be loaded automatically next time the GUI is started)
  • The Load button for fiducials got a more verbose label
  • Added a quite verbose description to help the user understand what fiducials are currently loaded and how to discover them in the figure
  • For writing fiducials, we now create a proper DigMontage (instead of a list of dicts, as was done previously) -> should be more robust!
  • _dock_add_file_button method gained two new parameters, filter to only allow filenames meeting the filter criteria, and initial_directory to point the file picker to the specified initial directory. I've adjusted the trans file pickers accordingly (loading / saving; initial directory for both is now the parent directory of the digitization source)
  • _dock_add_label now enables line wrapping
  • Dock width is limited to 350 pixels
  • Coregistration class gained a _fid_filename attribute to store the filename of the loaded fiducials file
  • Some documentation updates

This was more or less a quick hack to remove a blocker for @SophieHerbst and me.

Things that would need to be addressed in a followup PR:

  • Behavior when subject is changed. Currently, the previously-loaded fiducials remain loaded (albeit the label text changes and makes clear that they are "custom"). We'd need to factor out part of __init__() into a new method.
  • Right dock is too narrow for me on macOS. This is however not because the 350 pixel limit I set is too small per se, but we're somehow making bad use of the available space. We can do better!
  • I'd like to rework the subjects_dir picker as well.

I also discovered a bug: Loading a trans doesn't automatically apply it (can be seen in the last few seconds of the video) (fixed in #10224)

cc @GuillaumeFavelier

@hoechenberger hoechenberger marked this pull request as ready for review January 23, 2022 21:07
@SophieHerbst
Copy link
Contributor

That looks great @hoechenberger! Very explicit now.

hoechenberger added a commit to hoechenberger/mne-python that referenced this pull request Jan 24, 2022
Splitting the changes from mne-tools#10237 into smaller, separate PRs.
hoechenberger added a commit to hoechenberger/mne-python that referenced this pull request Jan 24, 2022
This has been extracted from mne-tools#10237 to split that PR into smaller, individual ones.
hoechenberger added a commit that referenced this pull request Jan 24, 2022
* Documentation improvements extracted from #10237

Splitting the changes from #10237 into smaller, separate PRs.

* Fix typo & phrasing [skip azp][skip actions]
@larsoner
Copy link
Member

I would not even allow "load from custom location". I would just have "load" and "save". If there are advanced users who really want to use custom locations, they can mv files as needed. I'd assume YAGNI on all the custom-location stuff

@hoechenberger
Copy link
Member Author

hoechenberger commented Jan 24, 2022

I would not even allow "load from custom location". I would just have "load" and "save". If there are advanced users who really want to use custom locations

Oh I thought this was a use case in your lab, which was actually the only reason why I added that 😅 But we did have a Load thingie before. So … shall I get rid of it? We auto-load anyway if the file can be found in the standard location.

@agramfort?

@larsoner
Copy link
Member

But we did have a Load thingie before. So … shall I get rid of it? We auto-load anyway if the file can be found in the standard location.

I would keep both load and save. Load allows resetting to the file-saved locations for example if you've done some clicking around to change the positions.

@hoechenberger
Copy link
Member Author

I would keep both load and save. Load allows resetting to the file-saved locations for example if you've done some clicking around to change the positions.

Ah ok, so we keep the ("a") Load button, but don't allow for custom files? We only (re)load from the standard location?

Sorry if these questions seem a little trivial, just trying to ensure there's no misunderstanding!

@larsoner
Copy link
Member

Yeah, I think just load/save buttons that operate on the standard location are probably good enough.

@hoechenberger
Copy link
Member Author

Yeah, I think just load/save buttons that operate on the standard location are probably good enough.

Got you! Have it working locally but I'm calling it a day for today. PR will be ready tomorrow.

@hoechenberger
Copy link
Member Author

Closing in favor of #10242

@hoechenberger hoechenberger deleted the coref-save-and-load branch January 26, 2022 16:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants