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

MAINT: Cleaner 3D vol viewer code #12570

Merged
merged 5 commits into from May 15, 2024
Merged

MAINT: Cleaner 3D vol viewer code #12570

merged 5 commits into from May 15, 2024

Conversation

larsoner
Copy link
Member

I really just wanted to take care of this:

    # XXX this assumes zooms are uniform, should probably mult by zooms...
    dist_to_verts = _DistanceQuery(stc_ijk)

by using the data in X/Y/Z / RAS / physical space instead of voxel space to do the distance query (which I did implement here). However, in working on it I realized there was a bug in my code that was very difficult to track down due to img being modified in the function, and me using it too early in the flow of the code.

This made me realize that it's not helpful to have a bunch of functions/callbacks defined inside the plot_volume_source_estimates code, because there's a mixture of namespaces that happens between those and the enclosing function. So I refactored the code by moving all those functions outside the function definition, then passing parameters that they need through the params dict. This should lead to more maintainable code.

The code is 98% copy-pasted. The remaining 2% changes are rewriting code to make use of params["img"] for example instead of img (which was formerly pulled from the enclosing function's variable namespace).

Based on how things were breaking in the tests when I went to fix the original # XXX comment, I think we can rely on them to tell us that things are still working okay after this PR. But I also touched a couple of tutorials that made use of the function so we can check to make sure they still look okay as well.

@larsoner
Copy link
Member Author

I don't think this really requires a changelog entry since there isn't really much user-facing stuff here. The differences between what we had before and how things behave now should be very minor for real-world use cases.

@larsoner larsoner added this to the 1.8 milestone May 14, 2024
@larsoner
Copy link
Member Author

@drammock this should be good to go now!

@drammock
Copy link
Member

tutorials look as they do on main. Thanks @larsoner !

@drammock drammock merged commit e43b5d8 into mne-tools:main May 15, 2024
30 checks passed
@drammock drammock deleted the refactor branch May 15, 2024 19:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants