[MRG] PCA flip for volumetric source estimates #13092

Shrecki · 2025-01-30T19:59:45Z

What does this implement/fix?

This PR implements PCA flip in volumetric source estimates.
To do so, it relies on using the already existing _pca_flip method. _pca_flip has been slighly modified to check if at least two vertices are available within a label. If that is not the case, it returns trivially the original signal, as PCA is ill-defined in this case.

Additional information

Feel free to have a look and - of course - double-check for correctness!

Extension of PCA flip in volumetric setting. Underlying logic is pretty much identical to cortical case with respect to flip vector creation and PCA itself Added a check in PCA flip in case the flip is None or number of vertices is below 2, in which case PCA will return a trivial estimate of the signal Complete with its own unit test case

…ate PR

Shrecki · 2025-01-31T14:24:32Z

I belive this PR is ready for review :) (although I do not know why pip pre is failing on Windows)

larsoner

Found some cruft, can you take a look at the diff on GitHub and make sure I found all of it?

Also with enhancements it's nice to see the effect in some example. Maybe you could modify this:

https://mne.tools/1.8/auto_examples/inverse/compute_mne_inverse_epochs_in_label.html

which comes from examples/inverse/compute_mne_inverse_epochs_in_label.py could be modified to add a volume example to show its flip modes as well? Or maybe some other example like tutorials/inverse/60_visualize_stc.py or examples/inverse/label_source_activations.py?

larsoner · 2025-01-31T19:30:37Z

mne/source_estimate.py

+    # logger.info("Selected mode: " + mode)
+    # print("Entering _prepare_label_extraction")
+    # print("Selected mode: " + mode)


Cruft, should be removed. But really to help next time:

Suggested change

# logger.info("Selected mode: " + mode)

# print("Entering _prepare_label_extraction")

# print("Selected mode: " + mode)

logger.debug(f"Selected mode: {mode}")

then if you run with verbose='debug' you will see the statement

Fixed with logger.debug, thanks!

larsoner · 2025-01-31T19:30:42Z

mne/source_estimate.py

@@ -3445,6 +3456,7 @@ def _prepare_label_extraction(stc, labels, src, mode, allow_empty, use_sparse):

    bad_labels = list()
    for li, label in enumerate(labels):
+        # print("Mode: " + mode + " li: " + str(li) + " label: " + str(label))


Suggested change

# print("Mode: " + mode + " li: " + str(li) + " label: " + str(label))

larsoner · 2025-01-31T19:30:49Z

mne/source_estimate.py

+                # print(f"src: {src[:2]}")
+                # print(f"len(src): {len(src[:2])}")
+


Suggested change

# print(f"src: {src[:2]}")

# print(f"len(src): {len(src[:2])}")

larsoner · 2025-01-31T19:33:58Z

mne/label.py

@@ -1460,22 +1460,47 @@ def label_sign_flip(label, src):
    flip : array
        Sign flip vector (contains 1 or -1).
    """
-    if len(src) != 2:
-        raise ValueError("Only source spaces with 2 hemisphers are accepted")
+    if len(src) > 2 or len(src) == 0:


A better / more modern check would be something like:

_validate_type(src, SourceSpaces, "src") _check_option("source space kind", src.kind, ("volume", "surface")) if src.kind == "volume" and len(src) != 1: raise ValueError("Only single-segment volumes, are supported, got labelized volume source space")

And incidentally I think eventually we could add support for segmented volume source spaces, as well as mixed source spaces (once surface + volume are fully supported, mixed isn't too bad after that). But probably not as part of this PR!

Probably! To support this, the (clarified) code in label_sign_flip with the hemis dictionary might be recycled:

hemis = {} # Build hemisphere info dictionary if label.hemi == "both": hemis["lh"] = {"id": 0, "vertno": src[0]["vertno"]} hemis["rh"] = {"id": 1, "vertno": src[1]["vertno"]} elif label.hemi in ("lh", "rh"): hemis[label.hemi] = {"id": 0, "vertno": src[0]["vertno"]} else: raise Exception(f'Unknown hemisphere type "{label.hemi}"')

larsoner · 2025-01-31T19:36:39Z

mne/label.py

+    if isbi_hemi:
+        lh_id = 0
+        rh_id = 1
+        lh_vertno = src[0]["vertno"]
+        rh_vertno = src[1]["vertno"]
+    elif label.hemi == "lh":
+        lh_vertno = src[0]["vertno"]
+    elif label.hemi == "rh":
+        rh_id = 0
+        rh_vertno = src[0]["vertno"]
+    else:
+        raise Exception(f'Unknown hemisphere type "{label.hemi}"')


I don't totally follow this logic. It's a surface source space if and only if it's bihemi, and it's volume source space if and only if it's a single-element list. I think it would be cleaner to triage based on source space type probably...

I've clarified it to be clearer:

hemis = {} # Build hemisphere info dictionary if label.hemi == "both": hemis["lh"] = {"id": 0, "vertno": src[0]["vertno"]} hemis["rh"] = {"id": 1, "vertno": src[1]["vertno"]} elif label.hemi in ("lh", "rh"): hemis[label.hemi] = {"id": 0, "vertno": src[0]["vertno"]} else: raise Exception(f'Unknown hemisphere type "{label.hemi}"') # get source orientations ori = list() for hemi, hemi_infos in hemis.items(): # When the label is lh or rh, get vertices directly if label.hemi == hemi: vertices = label.vertices # In the case where label is "both", get label.hemi.vertices # (so either label.lh.vertices or label.rh.vertices) else: vertices = getattr(label, hemi).vertices vertno_sel = np.intersect1d(hemi_infos["vertno"], vertices) ori.append(src[hemi_infos["id"]["nn"][vertno_sel]]) if len(ori) == 0: raise Exception(f'Unknown hemisphere type "{label.hemi}"')

mne/source_estimate.py

mne/tests/test_source_estimate.py

…pca_flip_volume

Shrecki · 2025-03-13T15:01:28Z

I believe the PR is ready for a second round :)

drammock · 2025-03-13T15:16:06Z

some test failures look related:

https://dev.azure.com/mne-tools/mne-python/_build/results?buildId=32675&view=logs&j=2bd7b19d-6351-5e7f-8417-63f327ab45bc&t=43b4c425-59e5-5cb3-0374-1f08e81b4191&l=5102

could you run these tests and debug locally? test_label_sign_flip, test_extract_label_time_course_volume_pca_flip, test_extract_label_time_course, test_extract_label_time_course_equiv, test_label_extraction_subject

Shrecki · 2025-03-14T10:55:31Z

All offending tests should now correctly be passing (locally no more issue on these tests)

mne/tests/test_source_estimate.py

larsoner · 2025-03-22T17:03:16Z

mne/tests/test_source_estimate.py

+    ],
+)
+def test_extract_label_time_course_volume_pca_flip(
+    src_volume_labels, label_type, mri_res, test_label, cf, call


We always want tests to be fairly complete, but competing against that we also want them to be as quick as possible. Our test suite takes over an hour to run already! 😢

Do we need to iterate over cf here for example? If we already test that functionality elsewhere and the pca_flip functionality here is independent of that, let's not. Same thing with mri_resolution, this seems like something that is unrelated and tested elsewhere most likely.

Maybe the pca_flip option could just be added somewhere else (maybe as one parameter in an already parametrized function) to really trim it down?

On this one, I'm on the fence - integrating pca_flip as an option in another test is certainly an option, much like "mean" for example, but at the same time we lose clarity in the testing if we over parameterize no? In particular, it becomes tedious to know what is actually being tested and which cases might or might not be covered...

In this case I think you're over-testing quite a bit. Your code change is only relevant to some minor parts of the code, not coordinate frame, mri resolution, etc. So I think it makes more sense to trim it down (a lot!) or integrate elsewhere

Shrecki added 5 commits October 15, 2024 12:04

example description update.

bcc07f1

pca_flip allowed for volumetric

99b593a

Merge remote-tracking branch 'upstream/main'

3b3b754

Merge branch 'main' into pca_flip_volume

d88591f

Shrecki requested review from drammock, agramfort and dengemann as code owners January 30, 2025 19:59

Shrecki added 2 commits January 31, 2025 10:42

Removed source_space_custom_atlas example - should be object of separ…

2e56dcb

…ate PR

[MISC] Changelog update

a677854

Shrecki requested a review from larsoner as a code owner January 31, 2025 09:56

Shrecki added 2 commits January 31, 2025 11:20

[MISC] Fixed changelog

8a74ffe

[FIX] Removed erroneous path from test case

dd15522

Shrecki changed the title ~~[FEAT] PCA flip for volumetric source estimates~~ [MRG] PCA flip for volumetric source estimates Jan 31, 2025

larsoner reviewed Jan 31, 2025

View reviewed changes

autofix-ci bot and others added 6 commits January 31, 2025 20:03

[autofix.ci] apply automated fixes

6839d73

[FEAT] Simplify label code and remove cruft code

379614e

Merge branch 'pca_flip_volume' of github.com:Shrecki/mne-python into …

d3523b7

…pca_flip_volume

[FIX] Removed trivial branch

33f911d

Merge remote-tracking branch 'upstream/main'

256331b

Merge branch 'main' into pca_flip_volume

1626e5f

[FIX] label_sign_flip incorrectly handled hemispheres

ec82986

Shrecki requested a review from larsoner March 18, 2025 16:24

larsoner reviewed Mar 22, 2025

View reviewed changes

Imports moved up top

0ca43cf

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MRG] PCA flip for volumetric source estimates #13092

[MRG] PCA flip for volumetric source estimates #13092

Shrecki commented Jan 30, 2025

Shrecki commented Jan 31, 2025

larsoner left a comment

larsoner Jan 31, 2025

Shrecki Mar 13, 2025

larsoner Jan 31, 2025

Shrecki Mar 13, 2025

larsoner Jan 31, 2025

Shrecki Mar 13, 2025

larsoner Jan 31, 2025

Shrecki Mar 13, 2025

larsoner Jan 31, 2025

Shrecki Mar 13, 2025

Shrecki commented Mar 13, 2025

drammock commented Mar 13, 2025

Shrecki commented Mar 14, 2025

larsoner Mar 22, 2025

Shrecki Mar 25, 2025

larsoner Apr 3, 2025

		# print(f"src: {src[:2]}")
		# print(f"len(src): {len(src[:2])}")

[MRG] PCA flip for volumetric source estimates #13092

Are you sure you want to change the base?

[MRG] PCA flip for volumetric source estimates #13092

Conversation

Shrecki commented Jan 30, 2025

What does this implement/fix?

Additional information

Shrecki commented Jan 31, 2025

larsoner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shrecki commented Mar 13, 2025

drammock commented Mar 13, 2025

Shrecki commented Mar 14, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment