Skip to content

Conversation

@JD-Zhu
Copy link
Contributor

@JD-Zhu JD-Zhu commented Oct 12, 2023

I've uploaded the data to OSF but not sure how to proceed. Would appreciate any help!

What does this implement/fix?

Add new dataset - KIT phantom data.

  • mne/datasets/phantom_kit/*.py to add some dataset functions
  • mne/datasets/config.py to add hashes and URLs
  • mne/datasets/utils.py to add it to our private download-all-data function
  • mne/datasets/__init__.pyi to add an import of the new data function
  • mne/utils/config.py to add a new config var for the path
  • .circleci/config.yml to cache the data
  • tools/circleci_download.sh to get CircleCI to download the data when the tutorial changes
  • doc/api/datasets.rst to document the new data_path function
  • doc/documentation/datasets.rst to briefly describe the dataset
  • doc/changes/latest.inc to mention your addition (ideally this PR would also include a tutorial
  • tutorials/inverse/95_phantom_kit.py that shows that source localization works with this dataset

@mscheltienne
Copy link
Member

mscheltienne commented Oct 12, 2023

Thank you, it looks like a good start, you'll also need to add the dataset information (URL, hash) to the configuration file https://github.com/mne-tools/mne-python/blob/main/mne/datasets/config.py and to the documentation build https://github.com/mne-tools/mne-python/blob/main/doc/api/datasets.rst

@larsoner
Copy link
Member

Looking back at a recent dataset addition d757026 I've edited your top comment to add checkboxes for the files that you'll want to make equivalent changes in (including config.py that @mscheltienne mentioned). There are a lot but looking at what's there hopefully it's easy enough to follow the patterns.

If you'd like help with this part don't hesitate to ask -- I can push a quick commit to do everything except adding the new tutorials/inverse/95_phantom_kit.py. Hopefully you can create that, look at existing tutorials/inverse/*phantom*.py files for examples!

@JD-Zhu JD-Zhu requested a review from larsoner as a code owner October 16, 2023 06:42
@JD-Zhu JD-Zhu requested a review from drammock as a code owner October 16, 2023 06:46
@JD-Zhu
Copy link
Contributor Author

JD-Zhu commented Oct 16, 2023

Looking back at a recent dataset addition d757026 I've edited your top comment to add checkboxes for the files that you'll want to make equivalent changes in (including config.py that @mscheltienne mentioned). There are a lot but looking at what's there hopefully it's easy enough to follow the patterns.

If you'd like help with this part don't hesitate to ask -- I can push a quick commit to do everything except adding the new tutorials/inverse/95_phantom_kit.py. Hopefully you can create that, look at existing tutorials/inverse/*phantom*.py files for examples!

Thanks a lot for the help! I've updated most of the files, except for "doc/overview/datasets_index.rst" which I couldn't locate.

Happy to try creating the tutorial - but I may need to come back and do this a bit later!

@larsoner
Copy link
Member

Looks like that file has moved to doc/documentation/datasets.rst

@JD-Zhu JD-Zhu marked this pull request as draft October 17, 2023 02:13
@JD-Zhu JD-Zhu marked this pull request as ready for review October 17, 2023 02:30
@JD-Zhu
Copy link
Contributor Author

JD-Zhu commented Oct 17, 2023

Thanks - now updated.

Some of the checks are failing and I'm not sure why - could you help me with these?

@JD-Zhu
Copy link
Contributor Author

JD-Zhu commented Oct 21, 2023

Thank you @larsoner !

I have fixed the login for CircleCI - please let me know if this still causes issues.

I'm still happy to attempt the analysis, but it might just take a while as I have quite a lot going on at the moment. To test things locally, how do I get it to run the version of code I'm working on rather than the mne version already installed on my computer?

Alternatively, if you are happy to do the analysis, that'd also be great! I think there are 49 dipoles activated in sequence within each "train", with each activation being a sinusoidal signal at 11Hz (see MISC 017). Please let me know if you require more information.

Do we need to get the "ground truth" dipole locations in order to verify the accuracy of source localisation?

@larsoner
Copy link
Member

It would improve the tutorial to know the true locations, yeah

Comment on lines 143 to 151
# We need to correct the ``dev_head_t`` because it's incorrect for these data!
# relative to the helmet: hleft, forward, up
translation = mne.transforms.translation(x=0.008, y=-0.025, z=-0.092)
# pitch down (rot about x/R), roll left (rot about y/A), yaw left (rot about z/S)
rotation = mne.transforms.rotation(
x=np.deg2rad(5),
y=np.deg2rad(-0.5),
z=np.deg2rad(-3),
)
Copy link
Member

Choose a reason for hiding this comment

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

@JD-Zhu the info['dev_head_t'] / MEG-to-head transformation for this dataset starts out as an identity matrix. This is incorrect, so I figured out some rotation and translation params by eye under the assumption that the phantom was symmetrical (which is probably a good assumption but by-eye adjustment is not great). Should we in principle be able to get a correct dev_head_t from this dataset, or was it not measured during acquisition? If it was measured, it suggests we need to add or fix something in MNE so that dev_head_t is not identity on read / analysis...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not too sure how to get the correct matrix - does this have to do with the marker coils and head shape digitisation? For KIT data, these are recorded in separate files (rather than being embedded in the raw MEG data). For this phantom dataset, we did marker coil measurements (_ini.mrk) but not head digitisation.

Copy link
Member

Choose a reason for hiding this comment

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

Ahh bummer. Yes I think you need both. If you still have the phantom and know where you had the coils you could maybe redigitize then update the dataset. But that sounds like a lot of work :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We no longer have the phantom unfortunately, but the marker coils were attached at fixed locations (i.e. there were designated pins on the phantom), so I think it may be possible to get the coordinates for these somehow.

Copy link
Member

Choose a reason for hiding this comment

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

Okay I think we can punt on this for now, then. We can always add these to the dataset at a later date if you track down the phantom and can do a marker measurement. Would be cool to see how close it is to my hand-optimized values. 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Haha yes, would be interesting to see!

Comment on lines +526 to +527
f"Trigger channel {ch_name} has a non-zero initial value of "
"{initial_value} (consider using initial_event=True to detect this "
Copy link
Member

Choose a reason for hiding this comment

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

While analyzing this dataset I initially tried mne.find_events on it and it wasn't immediately obvious to me what stim channel was actually being used, so I improved mne.find_events to log what it's actually using as the event channel.

[0, 0, 0, 1],
],
dtype=float,
)
Copy link
Member

Choose a reason for hiding this comment

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

I noticed this was identical to the code below so I DRY'ed it

@larsoner
Copy link
Member

@JD-Zhu based on the symmetry I'm assuming this is correct:

But it would be good to get the dipole locations if possible. And we should really fix the dev_head_t if possible as well (see comment above).

@JD-Zhu
Copy link
Contributor Author

JD-Zhu commented Oct 27, 2023

This is awesome! Thanks @larsoner

The dipole locations are available in this paper (see Table 3):
https://pubmed.ncbi.nlm.nih.gov/25985908/

@larsoner
Copy link
Member

The dipole locations are available in this paper (see Table 3):

Awesome, I'll look soon!

@larsoner larsoner closed this Oct 27, 2023
@larsoner larsoner reopened this Oct 27, 2023
@larsoner
Copy link
Member

Sorry, errant close!

* upstream/main: (26 commits)
  FIX: Fix bug with coreg scalars (mne-tools#12164)
  Changed casting rule in np.clip to allow reading of raw GDF files (mne-tools#12168)
  [DOC] Add documentation for setting montage order (mne-tools#12160)
  Fix inferring fiducials from EEGLAB (mne-tools#12165)
  Try to fix ICA Report (mne-tools#12167)
  BUG: Fix bug with Report.add_ica component number (mne-tools#12156)
  MAINT: Add rstcheck to CIs and pre-commit (mne-tools#12163)
  DOC: fix sphinx style typos (mne-tools#12161)
  MAINT: Fix linkcheck (mne-tools#12162)
  ENH: Add multiple label support to source_band_induced_power, source_induced_power (mne-tools#12026)
  Allow automated metadata generation to be bounded by "row events" instead of explicit time windows (mne-tools#12118)
  ENH: Collapse only in doc gen (mne-tools#12145)
  [pre-commit.ci] pre-commit autoupdate (mne-tools#12155)
  BUG: Fix bug with interior points not showing (mne-tools#12148)
  ENH: Warn about versions in sys_info (mne-tools#12146)
  Fix in conftest.py (mne-tools#12150)
  ENH: set color for bad channel with spatial_colors=True (mne-tools#12142)
  DOC: Better documentation of realign_raw (mne-tools#12135)
  Add mne-icalabel wildcard (mne-tools#12143)
  Remove LGTM.com configuration file (mne-tools#12139)
  ...
@larsoner
Copy link
Member

larsoner commented Nov 4, 2023

Okay I think this is good enough:

Average loc error: 1.7 mm
Average ori error: 9.6°

image

Just pushed one tiny commit to separate the fit_dipole output from the print(<dipole results info>) stuff so it's easier to see the average errors in the output.

Ready for review/merge from my end @drammock !

* upstream/main:
  ENH: Enable sensor-specific OPM coregistration in mne coreg (mne-tools#11405)
  Tweak README.rst (mne-tools#12166)
  [pre-commit.ci] pre-commit autoupdate (mne-tools#12177)
  MAINT: Add branch coverage (mne-tools#12174)
  OpenSSF (mne-tools#12175)
  fix docstring in 60_sleep.py (mne-tools#12171)
  FIX: skip empty lines in read_raw_eyelink (mne-tools#12172)
@larsoner larsoner added this to the 1.6 milestone Nov 7, 2023
Copy link
Member

@drammock drammock left a comment

Choose a reason for hiding this comment

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

looks great! just one nitpick, feel free to roll into a subsequent PR to save CI cycles if you want @larsoner

@larsoner larsoner merged commit 16f4411 into mne-tools:main Nov 14, 2023
@larsoner
Copy link
Member

Thanks @JD-Zhu !

snwnde pushed a commit to snwnde/mne-python that referenced this pull request Mar 20, 2024
Co-authored-by: Eric Larson <larson.eric.d@gmail.com>
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.

4 participants