Skip to content

Make ECoG and sEEG size defaults more realistic#12474

Merged
drammock merged 6 commits intomne-tools:mainfrom
libertyh:master
Mar 4, 2024
Merged

Make ECoG and sEEG size defaults more realistic#12474
drammock merged 6 commits intomne-tools:mainfrom
libertyh:master

Conversation

@libertyh
Copy link
Copy Markdown
Contributor

@libertyh libertyh commented Mar 1, 2024

Reference issue

Fixes issue #12472

What does this implement/fix?

Makes a minor change to the ECoG and sEEG default electrode sizes in defaults.py so they are more realistic (2e-3 instead of 5e-3). This makes it so sEEG and ECoG electrodes do not overlap one another and better reflect the actual size of electrodes in real devices.

Additional information

The original size was 5e-3 and new size is 2e-3. This reflects a 2mm contact size, which is closer to the typical 0.8-2.5 mm contact sizes for ECoG and sEEG. This also means that electrodes with spacing <5mm (which is quite typical) will not overlap in the visualizations.

@agramfort
Copy link
Copy Markdown
Member

@alexrockhill ok for you?

@libertyh
Copy link
Copy Markdown
Contributor Author

libertyh commented Mar 1, 2024

@alexrockhill ok for you?

If not, I can also go back to implementation (2) in issue #12472 and just have it as an option to pass electrode size instead. Thanks for any thoughts you have!

@agramfort
Copy link
Copy Markdown
Member

agramfort commented Mar 1, 2024 via email

@alexrockhill
Copy link
Copy Markdown
Contributor

Looks like a good default size to me! Glad you're using this @libertyh!

@drammock
Copy link
Copy Markdown
Member

drammock commented Mar 1, 2024

@libertyh please add a new file doc/changes/devel/12474.bugfix.rst with a changelog entry. View other files in that folder for formatting/content inspiration. (I'm suggesting calling this a bugfix and not enhancement based on "unrealistically large to the point of visual overlap").

never mind, I see you already did! I was misled by the failing changelog test, will have to see why that's happening.

@libertyh libertyh requested a review from drammock March 1, 2024 22:22
@drammock drammock enabled auto-merge (squash) March 4, 2024 19:44
@drammock
Copy link
Copy Markdown
Member

drammock commented Mar 4, 2024

test failures are unrelated --- we're testing against pre-release of NumPy 2.0 which has some API changes, and a dependency (pandas) hasn't yet accomodated. So I'll go ahead and merge; thanks @libertyh!

@drammock drammock disabled auto-merge March 4, 2024 20:31
@drammock drammock merged commit 795d6d9 into mne-tools:main Mar 4, 2024
snwnde pushed a commit to snwnde/mne-python that referenced this pull request Mar 20, 2024
Co-authored-by: Daniel McCloy <dan@mccloy.info>
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.

4 participants