Skip to content

[MRG, MAINT] Move freesurfer functions in mne/source_space.py to mne/_freesurfer.py#9543

Merged
larsoner merged 11 commits intomne-tools:mainfrom
alexrockhill:lut
Jul 12, 2021
Merged

[MRG, MAINT] Move freesurfer functions in mne/source_space.py to mne/_freesurfer.py#9543
larsoner merged 11 commits intomne-tools:mainfrom
alexrockhill:lut

Conversation

@alexrockhill
Copy link
Contributor

Helps with refactoring for #9520.

The freesurfer lookup table access seems oddly placed in source_space.py. I think utils.config.py makes sense because that is where get_subjects_dir is.

This would allow the lookup table to be accessed for plotting ROIs in other modules.

@alexrockhill
Copy link
Contributor Author

I don't think this should get a whatsnew entry since it's just a util but happy to do so if requested.

@larsoner
Copy link
Member

larsoner commented Jul 8, 2021

source_space.py is where a lot (most?) of our FreeSurfer mangling code lives. I agree source_space.py is at least a misnomer but we can't rename it for backward compat reasons -- perhaps we should make a new mne/_freesurfer.py or so rather than pack more stuff into utils, especially utils/config.py which should really just be about dealing with user config vars

@alexrockhill
Copy link
Contributor Author

source_space.py is where a lot (most?) of our FreeSurfer mangling code lives. I agree source_space.py is at least a misnomer but we can't rename it for backward compat reasons -- perhaps we should make a new mne/_freesurfer.py or so rather than pack more stuff into utils, especially utils/config.py which should really just be about dealing with user config vars

You know a lot better about where things go than I do, sounds good as far as naming.

@alexrockhill alexrockhill changed the title [MRG, MAINT] Expose freesurfer lookup table [MRG, MAINT] Move freesurfer functions in mne/source_space.py to mne/_freesurfer.py Jul 8, 2021
@alexrockhill
Copy link
Contributor Author

Ok @larsoner, this turned out to take a lot more changes than I thought. Now I get a circular import that get_subjects_dir is everywhere the basic import structure is off. I put an hour of work into it so if you want to advice on how to clean it up/clean it up I think this implements what you're suggesting. Otherwise we can just scale back if this is too much to do right now.

@larsoner
Copy link
Member

larsoner commented Jul 8, 2021

Let's just scale back, there is also a cost to moving things in terms of tracking blame and changes, and the tangible benefit is also small

@larsoner
Copy link
Member

larsoner commented Jul 9, 2021

... but now having read the PR's diff it is nice to have all this stuff in one place.

I'll see if I can push a commit to fix the circular import problem

@larsoner
Copy link
Member

larsoner commented Jul 9, 2021

I think the core problem is get_subjects_dir. That one I think actually makes at least as much sense to have in config.py as _freesurfer.py since it mostly uses get_config then does some value checks and raising (both of which are common in utils/ files) -- I think reverting those changes should fix things.

@agramfort
Copy link
Member

there is no public API change here?

@alexrockhill
Copy link
Contributor Author

there is no public API change here?

The only one would be that mne.utils.get_subjects_dir was moving to mne.get_subjects_dir but Eric put that back because of import issues so everything is exactly the same that is a public function in the API.

:toctree: generated/

get_mni_fiducials
coreg.get_mni_fiducials
Copy link
Member

Choose a reason for hiding this comment

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

is this an API change @alexrockhill

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That looks like it is the API that it was before, I just corrected it because it would have been a change but instead we just imported and noqa'd to shallow reference it in coreg

Copy link
Member

@agramfort agramfort left a comment

Choose a reason for hiding this comment

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

@larsoner merge if happy

thx @alexrockhill !

@larsoner larsoner merged commit 3b9ec94 into mne-tools:main Jul 12, 2021
@larsoner
Copy link
Member

Thanks @alexrockhill !

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