-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
[draft] new reader for curry files, using curry-python-reader code #13176
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
base: main
Are you sure you want to change the base?
Conversation
mne/io/curry/curryreader.py
Outdated
@@ -0,0 +1,633 @@ | |||
# Authors: The MNE-Python contributors. | |||
# License: BSD-3-Clause | |||
# Copyright the MNE-Python contributors. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this file the official reader? is this file copied from somewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, it's copied from https://github.com/neuroscan/curry-python-reader
the false info you flagged was added by [autofix.ci]
further formatting changes were necessary to pacify pre-commit hook
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you discussed this topic with a compumedics dev in #12855
they said they wont supply a pypi or conda version, but we are free to use the code.
the github repo has a BSD3 license applied, but they dont include any note in the file itself
Given that @CurryKaiser refused our offer to help them package up their reader for PyPI / conda-forge, I see two remaining options:
Personally I lean toward option 2. I say this because if we're going to try to support curry files, at a minimum we need to be able to fix bugs when they arise, and ideally we should be willing/able to incorporate new features that have high demand from our users ( |
hum... my first reaction is to push a version 0.1 of their package on pypi
and rely on this. Basically we maintain a fork and hope that the fork
changes are accepted upstream... it feels less hacky and they also have a
CI and some testing setup with test_data that I would not duplicate in
mne-python...
… Message ID: ***@***.***>
|
that indeed is less hacky than my approach to vendoring. I'd be OK with that outcome, though curious what @larsoner will think. |
I'm fine with that idea but it would be good to get some blessing/permission from them to do this |
xref to #12855 (comment) where I've asked for confirmation that Compumedics really doesn't want to be the packager and they're OK with us doing it. |
And nothing has changed, so all good from our side. Sorry we couldn't package it for you. And thank you for working on this! |
thanks @CurryKaiser ! ok, sounds like a plan.. I can start working on this again soon, if you give a go @agramfort @drammock @larsoner I guess the fork should live in the mne-tools org? I have the necessary rights to create it |
Yeah I think so |
I already made the fork |
ok, sounds like a plan.. I can start working on this again soon, if you
give a go @agramfort <https://github.com/agramfort> @drammock
<https://github.com/drammock> @larsoner <https://github.com/larsoner>Message
ID: ***@***.***>
+1
|
xref to mne-tools/curry-python-reader#1 |
i could need some guidance on 2 things:
a few other things to discuss:
|
p.s. and could you remind me how to switch off the CIs when pushing these early commits? |
Push commits with |
... for the cHPI stuff it's probably easiest to load a Neuromag raw FIF file with cHPI info and look at how the info is stored for example in For preload, since |
ok,
|
@CurryKaiser in another place you said you might be able to provide us with test files - could we perhaps get a small one with epoched recordings in it (format version shouldn't matter)? |
Could be that they were truncated, let me check. |
Ok, try these: |
thanks for the file @CurryKaiser |
@drammock @larsoner @agramfort currently related question: |
Yeah use |
see conda-forge PR: conda-forge/staged-recipes#29754 |
Ahh okay the comment from 2 weeks ago I read as meaning you were going to add to test data, modify code, etc. and then I assumed you would ping for review in another comment. Looks like you modified the code but didn't ping for review until today. Most maintainers don't get notified when commits are made for PRs -- it's waaaay too noisy -- so I didn't realize you had pushed. I should be able to look soon! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before I look at the MEG data (hopefully Monday), noticed a lot of red in the tests that I figured I'd ask about now
pytest.param(curry8_bdf_file, id="curry 8"), | ||
], | ||
) | ||
@pytest.mark.parametrize("mock_dev_head_t", [True, False]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was hoping existing tests would be touched/changed as little as possible... Can you explain why this test had to be removed? Or could it be put back and still work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
curryreader cannot read the c,rfDC
testfiles used in this and other tests.
they have more MEG sensor label, positions etc in the header file than channels in the data file (164 vs 148) and this case is explicitly set to raise in curryreader
.
so apparently this shouldnt happen -
i need to know more about these files.. can you give me some background?
- were these files created by some 3rd party software?
- can i just use some other test file, or is there a specific reason to use these files here?
- fwiw, it's possible to "fix" the files for reading by deleting the surplus channels from he header file. again, i'd need more information if this makes sense
"fname", | ||
[ | ||
pytest.param(curry_dir / "test_bdf_stim_channel Curry 7.cef", id="7"), | ||
pytest.param(curry_dir / "test_bdf_stim_channel Curry 8.cdt.cef", id="8"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same thing here (and below) ... why did this have to be removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the annotations test was just replaced by another test that does the same.
there are a few minor test that actually forgot to put back, though..
i did that and will push the commit once my fixes to the curryreader package are online (mne-tools/curry-python-reader#6)
@dominikwelke so this code, which uses
fails on ![]() So a couple of things are noticeable here. First, the helmet doesn't match the sensors. This is because the sensor defs are wrong:
This does not appear to be a Neuromag system I think (184 channels), so the Second, the
I haven't looked into what this means... but if there were 5 HPI coils, and they were defined in the head coordinate frame (5 points) and the MEG/device coordinate frame (same matched 5 points) then these can be used to get a proper |
yeah, this was a misunderstanding . i had repeatedly asked for review, with specific questions. no progress to be made without you actually looking into the code, so i just bumped this up again before leaving for conference travel :) thanks for having a look @larsoner! will comment on your points below |
noted.. that's the default coil
thanks for the lead. i cannot push the code yet, as i call the following - hpi_c/hpi_u are as from the
and get:
any idea how to get around this @larsoner ? |
|
this solved the issue directly following from that @larsoner - in the example case of
which is beyond both the default this currently breaks the reader as how to proceed?
fwiw, the sensor alignment looks better with this transform, but probably not great yet? idk if this is due to the sample mri.. |
curryreader 0.1.2 release is up: https://pypi.org/project/curryreader/ |
yikes, looks like the subject's nose is intersecting with the helmet surface! |
yeah, the nasion landmark definitely doesnt align with the sample subject's nasion :) |
just pushed the version with updated coil type and for you to make a call @larsoner :
on another note: |
Let's add a
I think there is a bug where the helmet surface is in the wrong place. If you look just at the MEG sensors (or plot with Can you paste a minimal example to reproduce the
I am not sure about these... can you browse through |
|
||
|
||
def _read_dig_montage_curry(ch_names, ch_types, ch_pos, landmarks, landmarkslabels): | ||
import re |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to nest this, should go at the top
rpa=landmark_dict["RPA"], | ||
hsp=hsp_pos, | ||
hpi=hpi_pos, | ||
coord_frame="head", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless you've transformed this to the head coord frame already (and it doesn't look like you have?) this should probalby be
coord_frame="head", | |
coord_frame="unknown", |
that way it'll transform everything to head coords using lpa/nasion/rpa
coord_frame="head", | ||
) | ||
else: # not recorded? | ||
warn("No eeg sensor locations found in file.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably should be an error if you try to read dig and can't
_check_curry_filename, | ||
_extract_curry_info, | ||
) | ||
from ._dig_montage_utils import _read_dig_montage_curry |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to nest this one
# for fname in [curry7_rfDC_file, curry8_rfDC_file]: | ||
# raw = read_raw_curry(fname, verbose=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# for fname in [curry7_rfDC_file, curry8_rfDC_file]: | |
# raw = read_raw_curry(fname, verbose=True) |
raw1 = read_raw_curry(fname, preload=False) | ||
raw1.load_data() | ||
assert raw1 == read_raw_curry(fname, preload=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you use just _test_raw_reader
it should automatically check preloading equivalence (that func has a test_preloading=True
default)
events, _ = events_from_annotations(raw, event_id=EVENT_ID) | ||
assert_allclose(events, REF_EVENTS) | ||
assert raw.info["dev_head_t"] is None | ||
assert not raw.info["dev_head_t"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change seems less explicit?
|
||
|
||
# In the new version based on curryreader package, time_step is always prioritized, i.e. | ||
# sfreq in the header file will be ignored and overridden by sampling interval |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we detect this mismatch and at least keep as a warning?
Does that mean that https://mne.tools/dev/auto_examples/visualization/meg_sensors.html#ctf is incorrect? Locally interacting with that 3D plot it looks OK (though I've not used a CTF system so just using instinct about what looks "reasonable") |
I think that one is okay... the sensors there look properly aligned with the helmet. I suspect the curry code modifies the MEG sensor coordinates or something -- at least compared to how we represent them when reading native files from the CTF system -- so then when we put the helmet in assuming the MEG sensors are in the same place, it's wrong. At least that's what I'm assuming is going on. Maybe a dumb question but are those CTF sensors? In the code we have |
hi all
as discussed in #13033 here a draft PR to use the official curry reader code.
in a first step i just use the reader as a module (only fixed formatting to force it past pre-commit).
it has some drawbacks (e.g. data is always loaded) and i did not implement all possible data yet (eg hpi, or epoched recordings) but in general it already works pretty well. tested it with all their example data, and one of my own recordings that didnt work in mne before.
it would be great to get some feedback how you want me to proceed with this @drammock @larsoner:
BACKGROUND:
the curry data reader currently cant handle all/newer curry files
plan is port code from the official curry reader into mne-python
for permission see #12855
closes #12795
closes #13033
closes #12855