Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MRG, ENH: Allow saving non-head cf in mon.save #8532

Merged
merged 2 commits into from Nov 16, 2020

Conversation

larsoner
Copy link
Member

As discussed with @agramfort, there is no need to have this restriction. Simultaneously makes things more DRY and removes two # XXX comments from our code, which is nice.

@agramfort agramfort merged commit 40872a8 into mne-tools:master Nov 16, 2020
4 checks passed
@agramfort
Copy link
Member

thx @larsoner !

@christian-oreilly
Copy link
Contributor

This won't work...

def write_dig(fname, pts, coord_frame=None):
    return _dig_write_dig(fname, pts, coord_frame=None)

@christian-oreilly
Copy link
Contributor

Also, after correcting the error mentioned below, the following code generate and assert exception

        montage = mne.channels.make_dig_montage(ch_pos=ch_pos,  
                                                nasion=brain_fiducials.loc["Nz"].values,
                                                lpa=brain_fiducials.loc["LPA"].values,
                                                rpa=brain_fiducials.loc["RPA"].values,
                                                hsp=hsp,
                                                coord_frame="mri")    

        montage_fname = fs_subject_path / subject / "{}-montage.fif".format(montage_str)
        
        mne.io.meas_info.write_dig(montage_fname, montage.dig, coord_frame=FIFF.FIFFV_COORD_MRI)
        dig = mne.channels.read_dig_fif(montage_fname).dig[:3]
        for d, want in zip(dig, (FIFF.FIFFV_POINT_LPA,
                                 FIFF.FIFFV_POINT_NASION,
                                 FIFF.FIFFV_POINT_RPA)):
            assert d['kind'] == FIFF.FIFFV_POINT_CARDINAL
            assert d['coord_frame'] == FIFF.FIFFV_COORD_MRI 
WRITING coord_frame in mne.io.write.write_dig_points 5

---------------------------------------------------------------------------
AssertionError                            Traceback (most recent call last)
<ipython-input-14-f785d8f255d5> in <module>
     71                                  FIFF.FIFFV_POINT_RPA)):
     72             assert d['kind'] == FIFF.FIFFV_POINT_CARDINAL
---> 73             assert d['coord_frame'] == FIFF.FIFFV_COORD_MRI
     74 
     75 

AssertionError: 

The printing of "WRITING coord_frame in mne.io.write.write_dig_points 5" indicates that the up to mne.io.write.write_dig_points the coordinate frame seems to be taken into account. Maybe the issue is in mne.channels.read_dig_fif ?

@christian-oreilly
Copy link
Contributor

Apologies. Please disregard the last message, I had not rebased my git when I tested. The previous one is valid though and this slight bug should be corrected for the full functionality of write_dig to work.

@larsoner
Copy link
Member Author

@christian-oreilly the idea of this PR is not use mne.io.meas_info.write_dig (this isn't really meant for public consumption despite its name), this should work fine if you just do montage.save(...) -- your assertions can pass. Can you see if using this more visible method works?

@christian-oreilly
Copy link
Contributor

Sure... I can check... but this code is still broken no?

def write_dig(fname, pts, coord_frame=None):
    return _dig_write_dig(fname, pts, coord_frame=None)

At least, if it is not considered broken, the documentation of write_dig should not say that coord_frame is a dummy attributes that do nothing.

@larsoner
Copy link
Member Author

Sure... I can check... but this code is still broken no?

Yes we should fix this

@christian-oreilly
Copy link
Contributor

FYI, montage.save(...) worked fine

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.

None yet

3 participants