Skip to content

Conversation

@alexrockhill
Copy link
Contributor

Fixes broken CI from #12382 (comment)

Ok so, I forgot to test that the shape was the same so volume source estimates with a different rr shape caused an error so I fixed that. But also, in doing so I noticed that the default, surface='pial' was being used (right). I'm not sure if this was on purpose but I think the surface=None version looks a lot better (left) which uses the src vertices. I think it actually was super confusing having an STC projected to pial surfaces and I would think maybe is a bug.

image

@alexrockhill
Copy link
Contributor Author

Failures are unrelated

@larsoner
Copy link
Member

Yes you shouldn't ever project to surface for a volume stc, the surf argument would be ignored completely in that case

@alexrockhill
Copy link
Contributor Author

Okay to merge to fix the error on main and then we can revisit if you want to really ignore surface for VolumeSourceEstimates and make it impossible to project to pial and not just not the default? I think the logic in this function could use some reworking, there's like 3 if src is/is not None and if surface is/is not None.

@larsoner larsoner merged commit ccf6794 into mne-tools:main Feb 12, 2024
@larsoner
Copy link
Member

Yes thanks @alexrockhill , feel free to improve the logic in another PR! It's likely volume/surface was not considered when writing this function, I think I wrote it (?) and only ever thought about ECoG projecting to surfaces

@alexrockhill alexrockhill deleted the fix branch February 12, 2024 17:26
snwnde pushed a commit to snwnde/mne-python that referenced this pull request Mar 20, 2024
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