@agramfort lmk if you think there's any issues still?

make it work with testing data as it contains all kinds of dig points you don't want to loose with get_montage

the test should do:
m = inst.get_montage()
inst.set_montage()
and inst should be unchanged

I used both the: fif_fname = op.join(io_dir, 'tests', 'data', 'test_raw.fif') and the bv_fname = op.join(io_dir, 'brainvision', 'tests', 'data', 'test.vhdr') along w/ the fif_dig_montage_fname = op.join(data_path, 'montage', 'eeganes07.fif') test datasets. I used a standard montage template, as well as one loaded from data. Note that the on_missing is required to allow full roundtrip of the montage setting.

FYI some issues w/ montage ordering?
I tried testing the actual montage received, but this doesn't work and I still am not sure why, but I think it's because the raw.info['dig'] does not maintain any ordering, so when you try to compare using _get_dig_montage_pos, the assertion doesn't work nicely.

    # XXX: dig montage doesn't maintain ordering...
    assert_array_equal(_get_dig_montage_pos(montage),
                        _get_dig_montage_pos(test_montage))

Codecov Report

Merging #7667 into master will increase coverage by 0.01%.
The diff coverage is 96.77%.

@@            Coverage Diff             @@
##           master    #7667      +/-   ##
==========================================
+ Coverage   90.36%   90.37%   +0.01%     
==========================================
  Files         455      455              
  Lines       84722    84780      +58     
  Branches    13425    13433       +8     
==========================================
+ Hits        76557    76621      +64     
+ Misses       5309     5300       -9     
- Partials     2856     2859       +3     

@adam2392 can you see why CIs complain? I'll review when it's green. thanks

@adam2392 can you see why CIs complain? I'll review when it's green. thanks

Sorry I totally dropped the ball on this, but will ping you when the CIs pass. Doing a check rn.

Okay I just fixed the codecov issue and now I think that once CI is green this is good to go assuming the changes I made addressed your comments sufficiently. (i.e. using the test dataset)

I woudl suggest we limit this PR to get_montage that returns channels that it make sense to use in set_montage (EEG, seeg, ecog, fnirs). I would leave the MEG out of this.

I woudl suggest we limit this PR to get_montage that returns channels that it make sense to use in set_montage (EEG, seeg, ecog, fnirs). I would leave the MEG out of this.

Sorry for the stupid questions (mne-python codebase is still a big one for me to understand), but... does what I have rn meet that? Since @larsoner suggested I just drop the orientations, I don't think I am doing it anymore(?) Or do I have to add an explicit picks somewhere in the get_ch_positions() function?

I think the idea is to get rid of get_ch_positions entirely and just have get_montage which returns positions only for the same set of channels that set_montage can set (EEG, sEEG, ECoG, not sure about fNIRS...)

Okay got rid of get_ch_positions and just left get_montage along w/ the related tests.

Not sure why the round trip for this test dataset is failing: fif_fname = op.join(io_dir, 'tests', 'data', 'test_raw.fif')

I get a mismatch say for example:

# original location of info['chs'][316]['loc']
[-0.01491282  0.11079782  0.07650087  0.          0.          0.
  0.          1.          0.          0.          0.          1.        ]

# new location of info['chs'][316]['loc']
[-0.01491282  0.11079782  0.07650087  0.00235201  0.11096951 -0.03500458
  0.          1.          0.          0.          0.          1.        ]

So I'm not sure if that's coming from the fact that there is a orientation change due to _get_data_as_dict_from_dig? However, the coordinates are the same...

I pushed up the changes so you can see locally. Sorry for the print statements in comments. I'll axe those when this is sorted out.

For EEG, first three entries in loc are the electrode pos, next three are the ref electrode pos (if present)

AFAIK we don't differentiate between different electrode types (EEG, ECoG, sEEG) in dig. In theory just the channel names matching in the montage instance should be good enough for those use cases.

Yes the standard order is fiducials/HPI (if present)/EEG/extra (headshape) points. So I think all that should need to be tweaked here really is ensuring that the ref gets ident==0 and is first in the list of EEG points...

Got it working by adding a "sorting" algorithm applied to the _format_dig_points function.

Thanks @adam2392 !