FIX: Preserve FIFF_NAME and fix active state handling in projection I/O#14029
FIX: Preserve FIFF_NAME and fix active state handling in projection I/O#14029sametSeckiiin wants to merge 5 commits into
Conversation
|
Hello! 👋 Thanks for opening your first pull request here! ❤️ We will try to get back to you soon. 🚴 |
nordme
left a comment
There was a problem hiding this comment.
Hi @sametSeckiiin ! Welcome -- we always appreciate PR's from new contributors!
I took a look at your code and have a couple thoughts about this fix. I think we should probably be preserving the original projector names as the descriptors. The information that's most useful to display when loading the raw file, drawn from the proj["desc"] field in the info, is actually the information in the FIFF.FIFF_NAME field in the original fif file, e.g. "PCA-v1-mag-permutated", "PCA-v2-mag-permutated", "PCA-v1-grad-permutated", etc., rather than "generated with dossp-2.1" for all projectors, which is what we get currently. @larsoner do you agree? I'm pretty sure back in the day we kept the projector names as the description.
If we want to preserve the projector names, then we'll need to alter the _read_proj function as well as the _write_proj function.
| write_int(fid, FIFF.FIFF_PROJ_ITEM_NVEC, proj["data"]["nrow"]) | ||
| write_int(fid, FIFF.FIFF_MNE_PROJ_ITEM_ACTIVE, proj["active"]) | ||
|
|
||
| if proj.get("desc") is not None: |
There was a problem hiding this comment.
Careful not to duplicate the name field in the saved fif file, which is what happens here. Also, note that we'll want to keep the fields in order.
There was a problem hiding this comment.
Thank you for catching that, @nordme!
I see the issue now—by adding the check there, I accidentally duplicated the name field and broke the expected field ordering in the FIFF structure.
I will inspect _write_proj to find where the name is originally written in the proper sequence, remove this duplicate check, and ensure that preserving proj["desc"] as FIFF_NAME is done exactly in the correct order (and coordinated with _read_proj). Working on the updated commits now
e885064 to
4089c75
Compare
for more information, see https://pre-commit.ci
|
Thanks again for the guidance, @nordme I have just pushed the updates: Removed duplicate write (_write_proj): Removed the redundant check at the end of the block to prevent duplicate tags and preserve standard FIFF field ordering. Preserved descriptor names (_read_proj): Updated the lookup priority to check FIFF_NAME before FIFF_DESCRIPTION, ensuring custom projector names are preserved across round-trip I/O. All 17 tests in test_proj.py are passing cleanly locally. Let me know if this looks good to merge |
|
@nordme and I took a look at an old FIFF file (recorded in 2020). What we see is that the projectors don't have a We think this is why the existing code checks first for a 206 field, and only then falls back to a 233 if the description is missing: Lines 588 to 594 in 9268e85 Comparing this to the screenshot in #13910, it looks like modern FIFFs are being written by MEGIN with both 233 and 206, and the info that used to be in 206 is now in 233. So we may want to triage based on which version of Megin software wrote the file. @nordme can you reach out to Megin and see if you can determine when this change happened and how best to triage? |
@drammock @sametSeckiiin Ok, I reached out to MEGIN, and I'll update here when I get the info!! |
|
@sametSeckiiin please don't idly merge main just because GitHub says "you're out of date with the base branch". merging main causes CIs to run, which uses energy and costs us money. Specifically in the situation we're in here: we know some more changes will need to be made but they haven't been made yet, so running CIs before those changes happen is wasteful. |
Fixes #13910
What does this implement/fix?
This PR addresses an I/O round-trip bug in
_write_proj(mne/_fiff/proj.py) where projection items lose metadata and force incorrect default values during saving.Specifically:
FIFF_NAME: Added a check to writeproj["desc"]asFIFF_NAMEduring saving so the projection name isn't dropped.activestate forcing0: Updatedwrite_intforFIFF_MNE_PROJ_ITEM_ACTIVEto only trigger whenproj["active"] is not None. Previously,Noneevaluated asFalsein basic truthy checks, which silently forced uninitialized states to0and altered the original data.Ran the projection test suite (
mne/tests/test_proj.py) locally. All 17 tests passed without any regression.