Skip to content

Conversation

@alexrockhill
Copy link
Contributor

Fixes #9577.

#9545 does this for DigMontages and so I think the only use case for this is Dipoles. There is already a to_mni function, I think there should just be an to_aseg function as well, that makes sense to me.

@larsoner
Copy link
Member

Do you need Dipole.to_aseg? I'd rather not add functionality until people have use cases that need to be met

@larsoner
Copy link
Member

(in other words, assume YAGNI until proven otherwise)

@alexrockhill
Copy link
Contributor Author

It's in the dipole tutorial that uses label.find_pos_in_annot/aseg. It's related to the mentioned issue.

This can be closed without merging but I made the changes and was going to keep it open for a minute until the issue is resolved or we just say it's not a big deal. I think it's actually a pretty big liability though because the coordinate frames are not that easy to understand.

@alexrockhill
Copy link
Contributor Author

Okay @larsoner, maybe take a quick look at this diff, it refactors find_label_in_annot/aseg to share _freesurfer.py code with #9545 and it makes the API much more consistent since there was already a dip.to_mni it makes sense to make a dip.to_mri.

@alexrockhill alexrockhill changed the title [ENH, WIP] Fix find aseg for Dipole [ENH, MRG] Fix find aseg for Dipole Jul 16, 2021
Copy link
Member

@larsoner larsoner left a comment

Choose a reason for hiding this comment

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

@mdovgialo is this API change okay with you?

@alexrockhill
Copy link
Contributor Author

Weird, the test passes locally for me and it hasn't been effected by the changes...

@alexrockhill
Copy link
Contributor Author

And it passed before I fixed the licenses...

@mdovgialo
Copy link
Contributor

mdovgialo commented Jul 18, 2021

@mdovgialo is this API change okay with you?

I'm fine with this

Copy link
Member

@larsoner larsoner left a comment

Choose a reason for hiding this comment

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

Other than the naming issue we should be good here, the failure is unrelated

* upstream/main:
  [MRG, ENH] Find aseg labels near montage (mne-tools#9545)
  Add label to colorbar in GAT plot [skip actions] (mne-tools#9582)
  [ENH, MRG] Encapsulate warp elec image in function (mne-tools#9544)
  [DOC, MRG] Add "info" to `docdict` (mne-tools#9574)
  [MRG] Add `units` parameter to get_data for Evoked (mne-tools#9578)
  [MRG, ENH] Get annotation description from snirf stim name (mne-tools#9575)
  [MRG] ENH, FIX: Add tmin/tmax parameters to get_data methods, fix bug in find_bads_ecg (mne-tools#9556)
@alexrockhill
Copy link
Contributor Author

Looks good to me :)

@larsoner larsoner merged commit f3a7dec into mne-tools:main Jul 19, 2021
@alexrockhill alexrockhill deleted the dipole branch July 19, 2021 16:58
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.

[BUG] mne.label.find_pos_in_annot doesn't check coord_frame

4 participants