Skip to content

ENH: Support TD data#11064

Closed
larsoner wants to merge 47 commits into
mne-tools:mainfrom
larsoner:feature/TD-nirs_snirf
Closed

ENH: Support TD data#11064
larsoner wants to merge 47 commits into
mne-tools:mainfrom
larsoner:feature/TD-nirs_snirf

Conversation

@larsoner
Copy link
Copy Markdown
Member

@larsoner larsoner commented Aug 21, 2022

Closes #9661

@rob-luke how about this plan:

For now I'm inclined to make it so if you get a SNIRF_PROCESSED data type that we don't know how to handle, we should maybe raise an error in this case...? For now I've removed it as a coil_type because this _PROCESSED really seems to mean something else specific, and we should perhaps add those one by one as we find use cases for them.

I have some collaborators who have seen Kernel help files telling them to use this very out of date branch, so it would be nice to get this in!

@larsoner larsoner added the fNIRS label Aug 23, 2022
@rob-luke
Copy link
Copy Markdown
Member

Thanks for jumping in to finish this off @larsoner

For now I'm inclined to make it so if you get a SNIRF_PROCESSED data type that we don't know how to handle, we should maybe raise an error in this case...?

Agreed

Ping me when you want a review, it doesn't need to be a final review, I am happy to take a look early if that helps.

@larsoner
Copy link
Copy Markdown
Member Author

Feel free to look now @rob-luke to see if you're happy with the overall design, the rest of the work is "just" some remaining details (adding unit tests, playing with the data a little bit to set scale factors, etc.) that I think we can rely on CIs to check, or that we can iterate on in follow-ups (e.g., tweaking plot scale factors)

Copy link
Copy Markdown
Member

@rob-luke rob-luke left a comment

Choose a reason for hiding this comment

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

Looking good, it will be great to have support for TD in MNE! I will review again once there are tests.

Comment thread mne/defaults.py Outdated
larsoner added 4 commits March 1, 2023 12:46
* upstream/main: (264 commits)
  BUG: Fix deprecated API usage in example (mne-tools#11512)
  Deprecate 'kind' and 'path' in favor of 'fname' in the layout reader (mne-tools#11500)
  EGI/MFF events outside EEG recording should not break the code (mne-tools#11505)
  fixed annotations error on export (mne-tools#11435)
  DOC: Update installer links [skip azp] [skip actions] [skip cirrus] (mne-tools#11506)
  BUG: updates for MPL 3.7 compatibility (mne-tools#11409)
  Fix docstrings by replacing str with path-like and fix double backticks for formatting (mne-tools#11499)
  Use pathlib.Path instead of os.path to handle files and folders [circle deploy] (mne-tools#11473)
  MAINT: Fix Circle [circle deploy] (mne-tools#11497)
  MAINT: Use mamba in CIs (mne-tools#11471)
  Updating documentation to clarify full vs half-bandwidth and defaults in time_frequency.multitaper.py (mne-tools#11479)
  Fix typo in tutorial (mne-tools#11498)
  Typo fix and added colons before code (mne-tools#11496)
  [MRG] ENH/DOC: demo custom spectrum creation (mne-tools#11493)
  Accept only left-clicks for adding annotations (mne-tools#11491)
  [BUG, MRG] Fix pial surface loading, logging in locate_ieeg (mne-tools#11489)
  [ENH] Added unit_role to add non-breaking space between magnitude  and units (mne-tools#11469)
  MAINT: Fix CircleCI build (mne-tools#11488)
  [DOC] Updated decoding.SSD documentation and internal variable naming (mne-tools#11475)
  Typo fix (mne-tools#11485)
  ...
Comment thread mne/defaults.py Outdated
Comment thread mne/defaults.py Outdated
Comment thread mne/defaults.py Outdated
Comment thread mne/io/snirf/tests/test_snirf.py Outdated
@larsoner
Copy link
Copy Markdown
Member Author

I am a bit busy for the next two weeks but then should be able to come back to this!

@larsoner larsoner modified the milestones: 1.9, 1.10 Dec 9, 2024
@larsoner
Copy link
Copy Markdown
Member Author

larsoner commented Jan 24, 2025

Okay two weeks turned into two months 😅 But I should be able to look next week

larsoner added 2 commits April 4, 2025 10:19
* upstream/main: (149 commits)
  FIX make_watershed_bem to handle missing talairach_with_skull.lta courtesy Freesurfer 8 (mne-tools#13172)
  ENH: Add upsampling for MEG helmet surface (mne-tools#13179)
  MAINT: Update code credit (mne-tools#13180)
  BUG: Fix bug with least-squares sphere fit (mne-tools#13178)
  fix EDF export (mne-tools#13174)
  fix typo (mne-tools#13171)
  [pre-commit.ci] pre-commit autoupdate (mne-tools#13164)
  Fix dev installation guide (mne-tools#13163)
  expose 'mode' for plotting dipole on brain (mne-tools#13162)
  turn dipole attrs into properties (mne-tools#13153)
  remove misformatted (and unused) crossref anchor (mne-tools#13155)
  doc: point to read_dipole (mne-tools#13149)
  [pre-commit.ci] pre-commit autoupdate (mne-tools#13152)
  BUG: Fix bug with not short-circuiting n_jobs=1 (mne-tools#13147)
  FIX: Missing coordinates.xml in MFF file (mne-tools#13148)
  FIX: Gracefully handle bad XML files in EGI reader (mne-tools#13145)
  Fixes for Latest IPython (9.0.1) (mne-tools#13146)
  Fix intersphinx (mne-tools#13143)
  BUG: Fix bug with parallel doc build (mne-tools#13140)
  [pre-commit.ci] pre-commit autoupdate (mne-tools#13141)
  ...
Comment thread mne/defaults.py
@larsoner
Copy link
Copy Markdown
Member Author

larsoner commented Apr 4, 2025

Apologies for the embarassingly long delay on this one 🥵 Pushing some commits to make sure things are green, then I'll add the test files, etc!

Copy link
Copy Markdown
Member Author

@larsoner larsoner left a comment

Choose a reason for hiding this comment

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

@julien-dubois-k okay I think I have it roughly where it could use another look from you to see if values are in the correct ranges, etc.!

Comment thread mne/io/snirf/_snirf.py
Comment thread mne/io/snirf/tests/test_snirf.py Outdated
assert hbo_data.shape == hbr_data.shape == (shape[0] // 2, shape[1])
hbo_norm = np.nanmedian(np.linalg.norm(hbo_data, axis=-1))
hbr_norm = np.nanmedian(np.linalg.norm(hbr_data, axis=-1))
# TODO: Old file vs new file scaling, one is wrong!
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Would be good to know if the numbers for the newer one are correct or not...

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

sorry, what do you mean?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The way the code works now the old test file and the new test file differ by several orders of magnitude (like 1e5) so I think there is uM vs M error scaling either in the file or in the code. Based on It looks like they should be closer to ~1e-6, right? If so then there is some bug in how we're reading the hbo/hbr values for the old file

Comment thread mne/io/snirf/tests/test_snirf.py Outdated
Comment on lines +484 to +485
# TODO: Reasonable values here???
lims = dict(intensity=(1e4, 1e7), mean=(1e3, 1e4), variance=(1e5, 1e7))
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I also don't know if these are reasonable values for the counts (intensity), mean (s), or variance (s**2)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

these are reasonable if mean is in picoseconds and variance in ps^2.

Comment thread mne/defaults.py Outdated
fnirs_td_moments_amplitude=1e6,
fnirs_td_gated_amplitude=1.0,
fnirs_td_moments_intensity=1.0,
fnirs_td_moments_mean=1.0,
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

the values are in the range of nanoseconds, so this should be 1e-9 for mean and 1e-18 for variance maybe?

larsoner added 2 commits June 26, 2025 14:30
* upstream/main: (55 commits)
  doc: fix rendering typo rst docstr (mne-tools#13301)
  DOC: fix docstrs around layout functions (mne-tools#13300)
  MAINT: Fix doc build failure due to deprecation (mne-tools#13299)
  Birthday input cast to datetime.date (mne-tools#13284)
  DOC: fix missing space, use f-strings, structure->object (mne-tools#13291)
  [pre-commit.ci] pre-commit autoupdate (mne-tools#13290)
  ENH: channel_indices_by_type now has an exclude param (mne-tools#13293)
  Proj id and proj name access (mne-tools#13261)
  Fix: nearly invisible traces with spatial_colors=True (mne-tools#13286)
  [pre-commit.ci] pre-commit autoupdate (mne-tools#13283)
  Bump autofix-ci/action from 551dded8c6cc8a1054039c8bc0b8b48c51dfc6ef to 635ffb0c9798bd160680f18fd73371e355b85f27 in the actions group (mne-tools#13282)
  fix Maxwell bads filtering (mne-tools#13280)
  fix actionable linkcheck errors (mne-tools#13273)
  MAINT: Use radius keyword with PyVista tube (mne-tools#13277)
  BUG: Fix bug with simulating head pos and BEM (mne-tools#13276)
  [pre-commit.ci] pre-commit autoupdate (mne-tools#13274)
  MAINT: Update code credit (mne-tools#13267)
  Annotations extras (mne-tools#13228)
  Tidy up the directory reading (mne-tools#13268)
  FIX, DOC: Drop bad channel in 10_publication_figure.py (mne-tools#13266)
  ...
@larsoner
Copy link
Copy Markdown
Member Author

Okay made some (again) overdue progress here! I think we need to look at / revise mne-tools/mne-nirs#586 next... see my comment mne-tools/mne-nirs#586 (comment)

@larsoner larsoner removed this from the 1.11 milestone Nov 13, 2025
@cbrnr cbrnr removed request for cbrnr and dengemann April 23, 2026 10:33
@jcrdubois
Copy link
Copy Markdown

@larsoner I added changes from this stale branch on top of master, and fixed a couple of things, here #13938 Let's test this and get this merged soon!

@larsoner
Copy link
Copy Markdown
Member Author

larsoner commented Jun 1, 2026

Closing for #13938

@larsoner larsoner closed this Jun 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Development

Successfully merging this pull request may close these issues.

5 participants