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] Support cross-talk and fine-calibration files #562

Merged
merged 13 commits into from Sep 27, 2020

Conversation

hoechenberger
Copy link
Member

@hoechenberger hoechenberger commented Sep 25, 2020

PR Description

Adds support for Elekta/Neuromag/MEGIN cross-talk and fine-calibration files. Closes #495.

  • mne_bids.write_cross_talk()
  • mne_bids.write_fine_calibration()
  • BIDSPath.cross_talk_fpath property*
  • BIDSPath.fine_calibration_fpath property*
  • CLI: mne_bids crosstalk_to_bids
  • CLI: mne_bids calibration_to_bids

BIDS spec:
https://bids-specification.readthedocs.io/en/latest/99-appendices/06-meg-file-formats.html#neuromagelektamegin

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>"

@hoechenberger
Copy link
Member Author

This is currently blocked by an upstream PR for bids-validator, bids-standard/bids-validator#1079

Copy link
Member

@sappelhoff sappelhoff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

API looks good to me, just two minor comments.

mne_bids/tests/test_write.py Outdated Show resolved Hide resolved
mne_bids/tests/test_path.py Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Sep 27, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@566cdbb). Click here to learn what that means.
The diff coverage is 91.66%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #562   +/-   ##
=========================================
  Coverage          ?   92.72%           
=========================================
  Files             ?       17           
  Lines             ?     2267           
  Branches          ?        0           
=========================================
  Hits              ?     2102           
  Misses            ?      165           
  Partials          ?        0           
Impacted Files Coverage Δ
mne_bids/config.py 91.17% <ø> (ø)
mne_bids/commands/mne_bids_calibration_to_bids.py 83.33% <83.33%> (ø)
mne_bids/commands/mne_bids_crosstalk_to_bids.py 83.33% <83.33%> (ø)
mne_bids/path.py 96.32% <100.00%> (ø)
mne_bids/write.py 95.80% <100.00%> (ø)

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 566cdbb...7d27576. Read the comment docs.

@hoechenberger
Copy link
Member Author

Hum, what's up with the CI now?!

@hoechenberger
Copy link
Member Author

All tests pass locally for me and the error message(s) we see here in the CI don't make any sense to me…

@hoechenberger
Copy link
Member Author

Should hopefully pass now after rebasing after #564 has been merged.

@hoechenberger
Copy link
Member Author

The tests that are failing now use the stable bids-validator, which doesn't yet support the changes implemented here – only the master branch of bids-validator currently does. How should we deal with this? @sappelhoff?

@hoechenberger
Copy link
Member Author

hoechenberger commented Sep 27, 2020

Ok so I've expanded the build matrix to use the stable and the development version of bids-validator on all platforms. All tests with the development version seem to pass now, so this should be good to merge.

@hoechenberger hoechenberger changed the title Support cross-talk and fine-calibration files [MRG] Support cross-talk and fine-calibration files Sep 27, 2020
@agramfort
Copy link
Member

@hoechenberger it would be better to skip test if bids_validator does not have the correct version. Merging a PR with red CI is bad.

examples/convert_mne_sample.py Outdated Show resolved Hide resolved
mne_bids/commands/mne_bids_crosstalk_to_bids.py Outdated Show resolved Hide resolved
mne_bids/path.py Outdated Show resolved Hide resolved
mne_bids/path.py Outdated Show resolved Hide resolved
mne_bids/commands/tests/test_cli.py Show resolved Hide resolved
mne_bids/write.py Outdated Show resolved Hide resolved
mne_bids/write.py Outdated Show resolved Hide resolved
@hoechenberger
Copy link
Member Author

Ugh now the tests are skipped on all platforms… because the bids-validator version number is not updated on GitHub master (is the same as in the latest stable release.) @sappelhoff is this intentional or do you think sb has just forgotten to update the version number?

@hoechenberger
Copy link
Member Author

Ugh now the tests are skipped on all platforms… because the bids-validator version number is not updated on GitHub master (is the same as in the latest stable release.) @sappelhoff is this intentional or do you think sb has just forgotten to update the version number?

I've opened an issue upstream, bids-standard/bids-validator#1080

@hoechenberger hoechenberger changed the title [MRG] Support cross-talk and fine-calibration files [WIP] Support cross-talk and fine-calibration files Sep 27, 2020
@hoechenberger hoechenberger changed the title [WIP] Support cross-talk and fine-calibration files [MRG] Support cross-talk and fine-calibration files Sep 27, 2020
@hoechenberger
Copy link
Member Author

bids-validator has been updated, and I've just restarted the CI jobs here and ensured that we're actually running the all tests (and not accidentally skipping some). All good, all green. Good to merge.

@agramfort agramfort merged commit 2bc1dbb into mne-tools:master Sep 27, 2020
@hoechenberger hoechenberger deleted the ct-calib branch September 27, 2020 18:24
@sappelhoff
Copy link
Member

@sappelhoff is this intentional or do you think sb has just forgotten to update the version number?

this is/was a known issue for a long time --> bids-standard/bids-validator#803

nobody got around to fixing it, and I am not sure whether your PR will persist, because I have no idea about the release process of bids-validator and what kinds of automations are in place that will overwrite/undo your changes - or break because of them 🙂

I pinged Ross (the maintainer) for that issue ... best case scenario, your simple fix was exactly what was needed --> let's hope for that.

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.

MEG fine-calibration and crosstalk files
4 participants