[WIP] Refactor write/read electrodes to a separate function & included coordinate system descriptions

[adam2392]

PR Description

Closes: #398

The main portion refactors writing and reading electrodes.tsv files into a _write_dig_bids and _read_dig_bids functions, and also includes coordinate system descriptions when writing coordsystem.json files. This is a move to help uncomplicate the read_raw_bids and write_raw_bids functions when dealing w/ electrode coordinates. Eventually, the _write/read_dig_bids funcs could add a public API even?

Summary
This is part 1 to address comments at: #416 (comment). This will move all related electrodes.tsv and coordsystem.json reading/writing files over to a dig.py. In addition, it makes some minor changes to the naming of the BIDS_COORDINATE_UNITS.

Once the simple "move of functions to dig.py" PR in #425 is merged, these are the things left to do:

• add coordinate system descriptions that are copied from the specification appendix. We can easily accommodate all the coordinate systems accepted by MNE-Python.
• refactor the read section (

  mne-bids/mne_bids/read.py

  Lines 554 to 555 in 47ad7dd

  	if electrodes_fname is not None:
  	if coordsystem_fname is None:

  ) to a _read_dig_bids() function and tidy up the logic
• refactor the write section (

  mne-bids/mne_bids/write.py

  Lines 1257 to 1258 in 47ad7dd

  	unit = "m" # defaults to meters
  	# We only write electrodes.tsv and accompanying coordsystem.json

  ) to a _write_dig_bids() function and tidy up the logic
• add scalp EEG back into _write_dig_bids(), only allowing writing if the landmarks (NAS, LPA, RPA) are present (I'm okay if you want me to table this one, but had this stored from a previous PR, so figured it's easy to add)
• update unit tests

Merge checklist

Maintainer, please confirm the following before merging:

• All comments resolved
• This is not your own PR
• All CIs are happy
• PR title starts with [MRG]
• whats_new.rst is updated
• PR description includes phrase "closes <#issue-number>"
• Commit history does not contain any merge commits

[adam2392]

@jasmainak lmk what you think? I think the file separation and function separation makes the whole electrodes/coordsystem business a lot cleaner. I added back in the scalp EEG only allowing the Captrak coordinate system as per previous discussions.

Added more configuration definitions which were from BIDS specification that we can get for free. E.g. the CoordinateSystemDescription field.

[jasmainak]

sorry for the delay @adam2392

@hoechenberger @sappelhoff would you mind taking a first pass? I'll look in a couple of days

[jasmainak]

@adam2392 the diff is huge. Part of the reason is that you have moved things around. Github doesn't do a good job of tracking changes when you move things. Plus it's difficult to review.

What I suggest is to break down the PR into two parts

1. Just move things around to make it easier to make concrete API changes.
2. Do the actual refactoring and API changes.

Ideally, 1. can be merged without much review and for 2., we will do the actual review. Do you think this is possible?

Also it would be really helpful if you can summarize in a concise comment what you moved where and any other changes that needed to be done.

[adam2392]

Also it would be really helpful if you can summarize in a concise comment what you moved where and any other changes that needed to be done.

@jasmainak

Summary
This is part 1 to address comments at: #416 (comment). This will move all related electrodes.tsv and coordsystem.json reading/writing files over to a dig.py. In addition, it makes some minor changes to the naming of the BIDS_COORDINATE_UNITS.

Once the simple "move of functions to dig.py" PR in #425 is merged, these are the things left to do:

• add coordinate system descriptions that are copied from the specification appendix. We can easily accommodate all the coordinate systems accepted by MNE-Python.
• refactor the read section (

  mne-bids/mne_bids/read.py

  Lines 554 to 615 in 47ad7dd

  	if electrodes_fname is not None:
  	if coordsystem_fname is None:
  	raise RuntimeError("BIDS mandates that the coordsystem.json "
  	"should exist if electrodes.tsv does. "
  	"Please create coordsystem.json for"
  	"{}".format(bids_basename))
  	# Get MRI landmarks from the JSON sidecar
  	with open(coordsystem_fname, 'r') as fin:
  	coordsystem_json = json.load(fin)
  	# Get coordinate frames that electrode coordinates are in
  	# Note: all coordinate frames should be lower-case
  	if kind == "meg":
  	coord_frame = coordsystem_json['MEGCoordinateSystem']
  	coord_unit = coordsystem_json['MEGCoordinateUnits']
  	elif kind == "ieeg":
  	space = coordsystem_json['iEEGCoordinateSystem']
  	coord_unit = coordsystem_json['iEEGCoordinateUnits']
  	# only compare lower case
  	space = space.lower()
  	coord_unit = coord_unit.lower()
  	# XXX: improve reading from 'other' systems
  	if space not in BIDS_IEEG_COORDINATE_FRAMES:
  	warn("{} Coordinate frame is not accepted BIDS "
  	"keyword for {}. Use CoordinateSystem with "
  	"one of these keywords: {}"
  	.format(space, bids_fname, BIDS_IEEG_COORDINATE_FRAMES))
  	coord_frame = None
  	elif space == 'acpc':
  	coord_frame = 'ras'
  	elif space == 'other':
  	elec_params = _parse_bids_filename(electrodes_fname, verbose)
  	coord_frame = elec_params['space']
  	# default coordinate frames to available ones in mne-python
  	if coord_frame not in MNE_IEEG_COORD_FRAME_DICT:
  	warn("Coordinate frame from coordinate system input {} "
  	"is still not supported. Reading in coordinate frame "
  	"as 'unknown'.".format(coord_frame))
  	coord_frame = 'unknown'
  	if coord_unit not in BIDS_IEEG_COORDINATE_UNITS:
  	warn("Coordinate unit is not an accepted BIDS unit for {}. "
  	"Please specify to be one of {}. Skipping electrodes.tsv "
  	"reading..."
  	.format(bids_fname, BIDS_IEEG_COORDINATE_UNITS))
  	coord_frame = None
  	else: # noqa
  	# XXX should add support of coordsystem.json for EEG
  	raise RuntimeError("Kind {} not supported yet for "
  	"coordsystem.json and "
  	"electrodes.tsv.".format(kind))
  	if coord_frame is not None:
  	coord_frame = coord_frame.lower() # MNE uses lower case
  	# read in electrode coordinates and attach to raw
  	raw = _handle_electrodes_reading(electrodes_fname, coord_frame,
  	coord_unit, raw, verbose)

  ) to a _read_dig_bids() function and tidy up the logic
• refactor the write section (

  mne-bids/mne_bids/write.py

  Lines 1257 to 1296 in 47ad7dd

  	unit = "m" # defaults to meters
  	# We only write electrodes.tsv and accompanying coordsystem.json
  	# if we have an available DigMontage
  	if raw.info['dig'] is not None:
  	# XXX: to improve writing with an encapsulated `_write_dig_bids`
  	if kind == "ieeg":
  	# get coordinate frame from digMontage
  	digpoint = raw.info['dig'][0]
  	# get the accepted mne-python coordinate frames
  	coord_frame_int = int(digpoint['coord_frame'])
  	space = MNE_VERBOSE_IEEG_COORD_FRAME.get(coord_frame_int,
  	None)
  	if space is not None:
  	# append the 'space' BIDs-entity to coordinates
  	coord_frame = "Other"
  	coordsystem_fname = make_bids_basename(
  	subject=subject_id, session=session_id,
  	acquisition=acquisition, space=space,
  	suffix='coordsystem.json', prefix=data_path)
  	electrodes_fname = make_bids_basename(
  	subject=subject_id, session=session_id,
  	acquisition=acquisition, space=space,
  	suffix='electrodes.tsv', prefix=data_path)
  	print("Writing to ", electrodes_fname)
  	# Now write the data for coords and the coordsystem
  	_electrodes_tsv(raw, electrodes_fname,
  	kind, overwrite, verbose)
  	_coordsystem_json(raw, unit, orient,
  	coord_frame, coordsystem_fname, kind,
  	overwrite, verbose)
  	else:
  	# default coordinate frame to mri if not available
  	warn("Coordinate frame of iEEG coords missing/unknown "
  	"for {}. Skipping writing "
  	"in of montage...".format(bids_fname))
  	elif kind != "meg":
  	logger.warning('Writing of electrodes.tsv '
  	'is not supported for kind "{}". '
  	'Skipping ...'.format(kind))

  ) to a _write_dig_bids() function and tidy up the logic
• add scalp EEG back into _write_dig_bids(), only allowing writing if the landmarks (NAS, LPA, RPA) are present (I'm okay if you want me to table this one, but had this stored from a previous PR, so figured it's easy to add)
• update unit tests

[agramfort]

@adam2392 I am lost. What should get in first? what is good to go from your end?

[adam2392]

@adam2392 I am lost. What should get in first? what is good to go from your end?

Sorry first time making such a large change...

#425 should go in first according to @jasmainak comments. This PR is just moving the existing code out of write.py and read.py. The next PR will address the remaining check list.

[jasmainak]

@adam2392 sounds like a plan, please update the PR description so that this is not lost in a comment.

[codecov-commenter]

Codecov Report

Merging #416 into master will decrease coverage by 0.10%.
The diff coverage is 85.58%.

@@            Coverage Diff             @@
##           master     #416      +/-   ##
==========================================
- Coverage   94.40%   94.29%   -0.11%     
==========================================
  Files          12       12              
  Lines        1537     1596      +59     
==========================================
+ Hits         1451     1505      +54     
- Misses         86       91       +5     

Impacted Files	Coverage Δ
mne_bids/dig.py	89.94% <83.15%> (-2.37%)	⬇️
mne_bids/config.py	95.00% <100.00%> (+0.71%)	⬆️
mne_bids/read.py	94.46% <100.00%> (+1.27%)	⬆️
mne_bids/write.py	97.76% <100.00%> (+0.16%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a1bad62...4cd9352. Read the comment docs.

[agramfort]

@jasmainak @sappelhoff please have a look before merging

[sappelhoff]

looks nice @adam2392 thanks a lot :-)

I left a few comments.

[sappelhoff]

also @adam2392 does this fix #203 ?

[adam2392]

also @adam2392 does this fix #203 ?

Addressed all the comments to the best I could. Regarding #203 , nope but it can in the future if I'm understanding correctly.

[sappelhoff]

okay, almost done from my side. Just three things I didn't notice in my first pass!

[agramfort]

thx @adam2392

we're making fast progress here !