[MRG][ENH]: Add Ability to export STC files as GIFTI #12309

pmolfese · 2023-12-18T23:58:14Z

Reference issue

What does this implement/fix?

Add functions under mne.export to handle exporting STC files as GIFTI. Should be easy to also expand from there to handle other surface file types if later desired. Given need to export both an "anatomical" and functional datasets, opted to not include in SourceEstimate / SurfaceEstimate classes and instead do what I felt was a cleaner option.

Additional information

Work in progress. Initial code stable, need to add tests and documentation. Opinions valued. Help of course is welcome!

larsoner · 2023-12-19T16:30:23Z

It is indeed probably long overdue to add something like this.

We already have VolSourceEstimate.as_volume for example, so maybe instead of mne.export.* it should be a mne.SourceEstimate method, maybe stc.export(...) that currently only supports gifti? If we ever add another format we can add a format="gifti" kwarg for example (but don't need to for the first PR.

pmolfese · 2023-12-19T17:14:38Z

Thanks @larsoner - I've updated the PR to move functionality to source_estimate. I went with save_as_surface as the function name to mirror save_as_volume. As you said, we can always refactor it slightly if there's ever a desire to support other formats. Still trying to reteach myself pytest and a little confused on how to generate docs for the new function. The contribution guide on mne.tools seems to be down (linked from https://mne.tools/stable/development/index.html)

drammock · 2023-12-19T17:17:08Z

The contribution guide on mne.tools seems to be down (linked from https://mne.tools/stable/development/index.html)

This one? https://mne.tools/stable/development/contributing.html It's linked in the left sidebar of the page you mentioned.

pmolfese · 2023-12-19T17:19:34Z

Thanks @drammock - not sure why I was laser focused on the in-line link in the text! Will check it out.

drammock · 2023-12-19T17:21:52Z

ah, oops, now I see the inline one is 404; will fix that. Thanks for letting us know.

…xportstc_gifti

into exportstc_gifti catching up

…t it existing since 1.0

into exportstc_gifti merging with current.

pmolfese · 2023-12-19T20:42:41Z

Getting stuck on how to generate documentation explicitly. I copied as closely as I could to save_as_volume but sphinx doesn't seem to be generating the necessary files to show the new function method. Any tips? Happy to do reading, just unsure of where to start.

larsoner · 2023-12-20T17:10:39Z

Can you go to https://app.circleci.com/pipelines/github/mne-tools/mne-python/22368/workflows/bd8128a6-9584-4d03-b606-810945e4c228/jobs/62531 and register/log in? Then CircleCI will build the doc and you can see how it looks

doc/changes/devel.rst

mne/source_estimate.py

mne/tests/test_source_estimate.py

Co-authored-by: Eric Larson <larson.eric.d@gmail.com>

version add Co-authored-by: Eric Larson <larson.eric.d@gmail.com>

Co-authored-by: Eric Larson <larson.eric.d@gmail.com>

pmolfese · 2023-12-20T18:47:24Z

@larsoner Thanks for all the input on the PR!

Getting a "block-unregistered-user" from that link after registering. Just a sanity check, do I need to be added to an authorized user list?

larsoner · 2023-12-20T18:51:16Z

That's odd, seems like it should work. I'll push a commit to get it to run and fix the merge conflict

* upstream/main: MAINT: Use towncrier for release notes (mne-tools#12299) MAINT: More [ci skip] MAINT: Add bot entry [ci skip] BUG: handle temporal discontinuities in Neuralynx `.ncs` files (mne-tools#12279) MAINT: Work around bad SciPy nightly wheels (mne-tools#12317)

larsoner · 2023-12-20T19:01:11Z

FYI I'd recommend running pre-commit install --install-hooks in your mne-python/ directory -- it should install some stuff that will auto-format when you git commit files. If you don't have pre-commit installed you can pip install pre-commit or similar first

pmolfese · 2023-12-20T19:08:10Z

Thanks @larsoner! I'll do that. I may make documenting all of these things part of my next PR to flush out the contribution docs so I don't forget!

larsoner · 2023-12-20T19:30:41Z

https://output.circle-artifacts.com/output/job/f365ffcb-fd32-407a-b216-04581d92e352/artifacts/0/html/generated/mne.SourceEstimate.html#mne.SourceEstimate.save_as_surface

pmolfese · 2023-12-20T19:42:39Z

Think we're ready. I could futz with it more to clean up some things but this is a win pre-holidays!

mne/source_estimate.py

larsoner

@drammock feel free to review/merge if you're happy!

drammock

nice and clean. +1 for merge (modulo my comments/questions about dtype, which I may be mistaken about).

There is a pip-pre error here (looks like NumPy changed an error message that broke our warnings filter) that we should address in a separate PR; I'll look tomorrow if @larsoner doesn't beat me to it.

doc/changes/devel/12309.newfeature.rst

mne/source_estimate.py

drammock · 2023-12-20T21:49:50Z

mne/source_estimate.py

+                    data=s["rr"] * scale_rr,
+                    intent="NIFTI_INTENT_POINTSET",
+                    datatype="NIFTI_TYPE_FLOAT32",


do we need to actually cast the data to float32 here? I read in nibabel docs that they don't actually enforce that the datatype matches the descriptor, but since we can make it match, seems like we should? (I'm assuming internally for us it's float64 right?)

I'll have to test it. I'm checking against the non-Python GIFTI world since that's my use case. We ran into issues where nilearn was writing float64 NIFTI files and that didn't cooperate well with the AFNI/FSL/SPM/Freesurfer programs. Perhaps we add it as an option to match?

For now let's just cast to and use float32. We can add float64 later if we really need it but I'd assume YAGNI until someone hits a use case where float32 isn't enough

drammock · 2023-12-20T21:51:07Z

mne/source_estimate.py

+                    data=s["tris"],
+                    intent="NIFTI_INTENT_TRIANGLE",
+                    datatype="NIFTI_TYPE_INT32",


similar comment as above about casting (here to int32, not float32)

mne/source_estimate.py

mne/tests/test_source_estimate.py

caps Co-authored-by: Daniel McCloy <dan@mccloy.info>

meth instead of class in diff doc Co-authored-by: Daniel McCloy <dan@mccloy.info>

caps in GIFTI Co-authored-by: Daniel McCloy <dan@mccloy.info>

Repetitive text Co-authored-by: Daniel McCloy <dan@mccloy.info>

larsoner

Will commit these once #12320 lands so CIs can come back green, then I'll mark for merge -- thanks in advance @pmolfese !

mne/source_estimate.py

Co-authored-by: Eric Larson <larson.eric.d@gmail.com> Co-authored-by: Daniel McCloy <dan@mccloy.info>

pmolfese added 2 commits December 18, 2023 18:37

add functions to export STC as GIFTI

b4d4bf4

added more attribution

0e4a0a8

move gifti export functionality to source_estimate.py

7c5b675

drammock mentioned this pull request Dec 19, 2023

fix 404 link on devel landing page #12316

Merged

pmolfese added 5 commits December 19, 2023 12:55

Merge branch 'main' into exportstc_gifti

42bdda6

Merge branch 'main' of https://github.com/mne-tools/mne-python into e…

c5ab7aa

…xportstc_gifti

devel doc update

dab0639

Merge branch 'exportstc_gifti' of https://github.com/pmolfese/mne-python

a0e19ab

into exportstc_gifti catching up

add test to write and check save_as_surface

1be89f1

pmolfese marked this pull request as ready for review December 19, 2023 19:56

pmolfese requested review from larsoner, drammock, agramfort and dengemann as code owners December 19, 2023 19:56

pmolfese added 3 commits December 19, 2023 14:58

Merge branch 'main' into exportstc_gifti

5ed43f3

Reusing existing get_decimated_surfaces from source_space, missed tha…

0e8960e

…t it existing since 1.0

Merge branch 'exportstc_gifti' of https://github.com/pmolfese/mne-python

0994c0e

into exportstc_gifti merging with current.

pmolfese added 2 commits December 19, 2023 16:17

styling fixes

1784e66

verbose breakage

7eaafcc

larsoner reviewed Dec 20, 2023

View reviewed changes

doc/changes/devel.rst Outdated Show resolved Hide resolved

mne/source_estimate.py Outdated Show resolved Hide resolved

mne/source_estimate.py Show resolved Hide resolved

mne/tests/test_source_estimate.py Show resolved Hide resolved

pmolfese and others added 3 commits December 20, 2023 13:11

Update doc/changes/devel.rst

070f9bb

Co-authored-by: Eric Larson <larson.eric.d@gmail.com>

Update mne/source_estimate.py

8b8f766

Co-authored-by: Eric Larson <larson.eric.d@gmail.com>

Update mne/source_estimate.py

8613719

version add Co-authored-by: Eric Larson <larson.eric.d@gmail.com>

Update mne/tests/test_source_estimate.py

f21209e

Co-authored-by: Eric Larson <larson.eric.d@gmail.com>

TST: Ping

e10f5d7

larsoner added 2 commits December 20, 2023 13:52

STY: pre-commit

1587da1

larsoner added 3 commits December 20, 2023 14:06

STY: Caps and dots

f205937

FIX: Name

bca2773

FIX: No nest

dffd5a9

pmolfese changed the title ~~[WIP] Add Ability to export STC files as GIFTI~~ [MRG][ENH]: Add Ability to export STC files as GIFTI Dec 20, 2023

MAINT: DRY code

11d77df

larsoner reviewed Dec 20, 2023

View reviewed changes

mne/source_estimate.py Show resolved Hide resolved

larsoner approved these changes Dec 20, 2023

View reviewed changes

drammock approved these changes Dec 20, 2023

View reviewed changes

pmolfese and others added 4 commits December 20, 2023 17:29

Update mne/tests/test_source_estimate.py

e6b7b0a

caps Co-authored-by: Daniel McCloy <dan@mccloy.info>

Update doc/changes/devel/12309.newfeature.rst

86a3429

meth instead of class in diff doc Co-authored-by: Daniel McCloy <dan@mccloy.info>

Update mne/source_estimate.py

a0df09d

caps in GIFTI Co-authored-by: Daniel McCloy <dan@mccloy.info>

Update mne/source_estimate.py

8a0b922

Repetitive text Co-authored-by: Daniel McCloy <dan@mccloy.info>

larsoner reviewed Dec 21, 2023

View reviewed changes

mne/source_estimate.py Outdated Show resolved Hide resolved

mne/source_estimate.py Outdated Show resolved Hide resolved

Apply suggestions from code review

d78b121

larsoner reviewed Dec 21, 2023

View reviewed changes

mne/source_estimate.py Outdated Show resolved Hide resolved

Update mne/source_estimate.py

c3d0075

larsoner enabled auto-merge (squash) December 21, 2023 15:37

larsoner merged commit a03a40d into mne-tools:main Dec 21, 2023
28 checks passed

pmolfese deleted the exportstc_gifti branch December 29, 2023 13:31

snwnde pushed a commit to snwnde/mne-python that referenced this pull request Mar 20, 2024

[MRG][ENH]: Add Ability to export STC files as GIFTI (mne-tools#12309)

d609c14

Co-authored-by: Eric Larson <larson.eric.d@gmail.com> Co-authored-by: Daniel McCloy <dan@mccloy.info>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MRG][ENH]: Add Ability to export STC files as GIFTI #12309

[MRG][ENH]: Add Ability to export STC files as GIFTI #12309

pmolfese commented Dec 18, 2023

larsoner commented Dec 19, 2023

pmolfese commented Dec 19, 2023

drammock commented Dec 19, 2023 •

edited

Loading

pmolfese commented Dec 19, 2023

drammock commented Dec 19, 2023

pmolfese commented Dec 19, 2023

larsoner commented Dec 20, 2023

pmolfese commented Dec 20, 2023

larsoner commented Dec 20, 2023

larsoner commented Dec 20, 2023

pmolfese commented Dec 20, 2023

larsoner commented Dec 20, 2023

pmolfese commented Dec 20, 2023

larsoner left a comment

drammock left a comment

drammock Dec 20, 2023

pmolfese Dec 20, 2023

larsoner Dec 20, 2023

drammock Dec 20, 2023

larsoner left a comment

[MRG][ENH]: Add Ability to export STC files as GIFTI #12309

[MRG][ENH]: Add Ability to export STC files as GIFTI #12309

Conversation

pmolfese commented Dec 18, 2023

Reference issue

What does this implement/fix?

Additional information

larsoner commented Dec 19, 2023

pmolfese commented Dec 19, 2023

drammock commented Dec 19, 2023 • edited Loading

pmolfese commented Dec 19, 2023

drammock commented Dec 19, 2023

pmolfese commented Dec 19, 2023

larsoner commented Dec 20, 2023

pmolfese commented Dec 20, 2023

larsoner commented Dec 20, 2023

larsoner commented Dec 20, 2023

pmolfese commented Dec 20, 2023

larsoner commented Dec 20, 2023

pmolfese commented Dec 20, 2023

larsoner left a comment

Choose a reason for hiding this comment

drammock left a comment

Choose a reason for hiding this comment

drammock Dec 20, 2023

Choose a reason for hiding this comment

pmolfese Dec 20, 2023

Choose a reason for hiding this comment

larsoner Dec 20, 2023

Choose a reason for hiding this comment

drammock Dec 20, 2023

Choose a reason for hiding this comment

larsoner left a comment

Choose a reason for hiding this comment

drammock commented Dec 19, 2023 •

edited

Loading