Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MRG: Rework CoregistrationUI fiducial I/O widgets & add save button #10242

Merged

Conversation

hoechenberger
Copy link
Member

@hoechenberger hoechenberger commented Jan 25, 2022

This is the last remaining bit of the #10237 rework, incorporating suggestions and feedback I've meanwhile received.

Closes #10237

I've removed the _fiducials_file traitlet as it's not needed anymore. I've moved its functionality to other methods.

Screen.Recording.2022-01-25.at.10.02.50.mov
  • Saving only allowed once fiducials are locked
  • Reloding fiducials automatically locks fiducials

There's a bug related to the digpoints still showing even though the fiducials are unlocked. I've left an extensive comment in the source. This is probably something that needs to be solved in a separate PR unless @GuillaumeFavelier has an immediate idea? At any rate, I don't think this PR introduced this problem 😅

cc @agramfort

@hoechenberger hoechenberger marked this pull request as ready for review January 25, 2022 09:11
@GuillaumeFavelier
Copy link
Contributor

I have multiple questions/remarks:

  • Why _fiducials_file is not needed anymore? because if that is the case, we have to instantiate normally and not use traitlets at all (I think it's still created by it when I looked)
  • I would avoid using lambda as callback (they do not work as you expect), try to use partial instead (maybe the bug is related to that)
  • I recommend using the _forward_widget_command() function as much as possible, it checks that widget exists (instead of _widgets[""] directly)
  • I saw we have another GUI API change with selectable in _dock_add_label, can we add a test for it?

@hoechenberger
Copy link
Member Author

  • Why _fiducials_file is not needed anymore? because if that is the case, we have to instantiate normally and not use traitlets at all (I think it's still created by it when I looked)

We call _set_fiducials_file() on reload button press and when switching subjects. The attribute _fiducials_file is still used, but it doesn't need to be a traitlet watching for events anymore. The functionality of the traitlet has been integrated into _set_fiducials_file()

  • I would avoid using lambda as callback (they do not work as you expect)

Why's that?

  • I recommend using the _forward_widget_command() function as much as possible, it checks that widget exists (instead of _widgets[""] directly)

I wasn't aware of this one, I'll have a look.

  • I saw we have another GUI API change with selectable in _dock_add_label, can we add a test for it?

Absolutely! Will do.

@GuillaumeFavelier
Copy link
Contributor

Why's that?

When I designed the callbacks to set translation and rotation values for example, I used a for-loop to iterate over the modes (either 'translation' or 'rotation'). So one of the input parameter of the callback is a local variable corresponding to the current mode. I would expect the instantiated lambda to contain the current value as well but I noticed that it holds only the last (basically all the callbacks were only modifiying rotation). I dunno if this is clear but for me, it is an unexpected behaviour I observed with lambda and local variables. I personally fixed this issue by using partial so that's why I mean there is no 'surprise' with it.

In your case though, you do not use local variables but attributes of the class, it should be okay but I did not test your branch yet.

@hoechenberger
Copy link
Member Author

In your case though, you do not use local variables but attributes of the class, it should be okay but I did not test your branch yet.

Ah yes, I see what you mean. Yes this is specifically what I ensured before choosing to use lambda. But I will switch to partial just to be on the safe side and consistent with the rest of the code. Thanks for the explanation!

hoechenberger added a commit to hoechenberger/mne-python that referenced this pull request Jan 25, 2022
hoechenberger added a commit to hoechenberger/mne-python that referenced this pull request Jan 25, 2022
hoechenberger added a commit to hoechenberger/mne-python that referenced this pull request Jan 25, 2022
hoechenberger added a commit that referenced this pull request Jan 25, 2022
GuillaumeFavelier pushed a commit that referenced this pull request Jan 26, 2022
…0245)

* Use overwrite='read' when checking for file to read

* Picking fiducials -> Placing MRI fiducials

* Limit dock width to 350px

* Set file filter and initial directory for trans file pickers
@hoechenberger hoechenberger marked this pull request as draft January 26, 2022 12:56
mne/gui/_coreg.py Outdated Show resolved Hide resolved
hoechenberger added a commit to hoechenberger/mne-python that referenced this pull request Jan 27, 2022
Partially extraced from mne-tools#10242 and mne-tools#10224

- status bar is now stored in `self._widgets`
- `_forward_widget_command()` has a docstring and new functionality
  (extracted from mne-tools#10224)
GuillaumeFavelier pushed a commit that referenced this pull request Jan 27, 2022
Partially extraced from #10242 and #10224

- status bar is now stored in `self._widgets`
- `_forward_widget_command()` has a docstring and new functionality
  (extracted from #10224)
mne/gui/_coreg.py Outdated Show resolved Hide resolved
@hoechenberger hoechenberger marked this pull request as ready for review January 28, 2022 08:06
@hoechenberger
Copy link
Member Author

hoechenberger commented Jan 28, 2022

@GuillaumeFavelier I think this one is good to merge now! Basically what we cooked up together yesterday, plus some really minor changes and fixing the bugs we encountered (except for the DigPoints showing with unlocked fiducials when switching subjects)

@hoechenberger hoechenberger changed the title Rework CoregistrationUI fiducial I/O widgets & add save button MRG: Rework CoregistrationUI fiducial I/O widgets & add save button Jan 28, 2022
Copy link
Contributor

@GuillaumeFavelier GuillaumeFavelier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is my only nitpick @hoechenberger

About the bug, I will update #8833 👍 For now, the temporary workaround is to lock then unlock the fiducials :(

mne/gui/_coreg.py Outdated Show resolved Hide resolved
@hoechenberger hoechenberger merged commit 89650ca into mne-tools:main Jan 28, 2022
@hoechenberger hoechenberger deleted the coreg-ui-fiducial-io-widgets branch January 28, 2022 11:12
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.

2 participants