Skip to content

Conversation

@georgeoneill
Copy link
Contributor

@georgeoneill georgeoneill commented Mar 7, 2023

Fixes #10780.

What does this implement/fix?

To do

  1. Add unit test(s)
  2. update OPM preprocessing tutorial to incorporate HFC (results will look way better I think).
  3. ?

Sorry this has taken this long to get around to this, I've been spinning a lot of plates recently, but this initial PR should keep this in the forefront of my mind for a while!

@larsoner
Copy link
Member

larsoner commented Mar 7, 2023

Could you run HFC (of different orders, e.g., maybe just 1, 2, and 3) using your MATLAB code on the mne-testing-data dataset we have for read_raw_fil? Then we can check the Python implementation to numerical precision :)

@georgeoneill
Copy link
Contributor Author

$\ell=1$

Max absolute difference: 1.56248141e-17
Max relative difference: 5.95234384e-08

$\ell=2$

Max absolute difference: 1.56249363e-17
Max relative difference: 5.95987598e-08

$\ell=3$

Max absolute difference: 4.2325848e-15
Max relative difference: 551.81531484

Interesting that $\ell=3$ has such a large difference. The lower orders all pass the default np.testing.assert_allclose(). This requires some investigation.

@larsoner
Copy link
Member

larsoner commented Mar 8, 2023

The lower orders all pass the default np.testing.assert_allclose(). This requires some investigation.

For this sort of thing I often boil it down to a single use case to test (sounds like you're already there!) and then

  1. save ... in MATLAB at some suitable place in the execution
  2. In Python insert a scipy.io.loadmat(...)['whatever'] and use the MATLAB array in place of the Python one
  3. Repeat 1-2 starting very late in the process (e.g., here, using the pseudo-inverted matrix) where you get equivalence, iteratively save .../loadmating earlier in the process until you find the culprit.

You can do the same thing by manually inspecting variables but that's not always easier...

@georgeoneill
Copy link
Contributor Author

Below are the results for the three orders when for varying amount of field correction computation done in MNE.

Top table is max absolute error, bottom is max relative error. The numbers correspoding to Basis & Projector are smaller than the values posted last week due to a way I save out the data involving fewer casting steps.

MAX ABS DIFFERENCE

Order Basis & Projector Projector Only None
$\ell=1$ 1.28277618e-24 1.04366929e-24 5.62224323e-25
$\ell=2$ 9.919705e-25 8.1748709e-25 6.33310156e-25
$\ell=3$ 4.23163525e-15 1.04690046e-24 5.68686671e-25

MAX REL DIFFERENCE

Order Basis & Projector Projector Only None
$\ell=1$ 1.14841825e-08 9.00876775e-09 3.62863866e-09
$\ell=2$ 2.57761766e-08 2.47442535e-08 5.75180087e-09
$\ell=3$ 551.81529938 1.06555737e-08 8.69080621e-09

Currently pointing to an unexpected divergence between _sss_basis and spm_opm_vslm.

@georgeoneill
Copy link
Contributor Author

I'm going to see what happens if we tell MNE to assume these are point magnetometers. Will report back.

@larsoner
Copy link
Member

Wait the max relative difference is now 1e-9? That seems fine to me! I mean ideally for double float we'd end up at something closer to 1e-12 but 1e-9 should be good enough (especially since we routinely save to disk in single precision which will lead to ~1e-6 relative errors anyway)

@larsoner
Copy link
Member

I'm going to see what happens if we tell MNE to assume these are point magnetometers. Will report back.

This could definitely be it, so feel free. But as above I think 1e-9 relative error is already enough. We routinely check for equivalence with MNE-C inverses for example at around 1e-7 relative precision or so

@georgeoneill
Copy link
Contributor Author

Wait the max relative difference is now 1e-9? That seems fine to me! I mean ideally for double float we'd end up at something closer to 1e-12 but 1e-9 should be good enough (especially since we routinely save to disk in single precision which will lead to ~1e-6 relative errors anyway)

For $\ell=3$ the max relative difference 1e-9 when you use the basis set from SPM, but as soon as you calculate the basis set in MNE the relative error becomes 1e3, which equates to femtoTesla level differences.

@larsoner
Copy link
Member

but as soon as you calculate the basis set in MNE the relative error becomes 1e3, which equates to femtoTesla level differences.

Ahh okay great that you isolated it!

BTW there has always been a slight mismatch when regularizing the maxwell filtering (SSS) basis set between what MNE chooses and what MaxFilter by MEGIN chooses. I wonder if we have some slight lingering scale factor or sign bug that changes things slightly... let me know what you find.

@larsoner
Copy link
Member

(Also in maxwell_filter we column normalize the bases so scale factor differences might not matter there, whereas they could for this function)

@georgeoneill
Copy link
Contributor Author

Its the coil definition!

Accurate max rel error: 551.81529938

image

Normal max rel error: 576.00884121

image

Point Magnetometer max rel error: 1.88025786e-07

image

@larsoner
Copy link
Member

@georgeoneill awesome!

A bit scary that the coil def matters this much. But in theory at least MNE has a better definition I think (?), so maybe we're good here.

If you want, in tests you could set the coil type manually to be a point magnetometer and test against some hard-coded MATLAB values that you either write in ASCII (ouch) or add to MNE-testing-data. Then you can have a nice satisfying rtol=1e-8 or so in tests.

@georgeoneill
Copy link
Contributor Author

I'll have a think about how we proceed with this. I'm not entirely sure how well definied the accurate coil definitions for QuSpin sensors actually are. There's the further complication for the triaxial sensors that there is a second laser used to generate the third axis so the 'accurate' coil definition may not entirely reflect the volume we should be integrating over.

I'll probably add a custom point magnetometer function to go with the accurate coil definition found in _prep_mf_coils (that we we dont need to touch mne.forward) with a flag which defaults to point magnetometer behaviour... for now.

+ Add the ability to choose a point magnetometer definition instead of accurate definition.
+ Add load/save functionality
+ Add EOGRegression-like behaviour
@georgeoneill
Copy link
Contributor Author

georgeoneill commented Mar 16, 2023

Still have the tests to make but wanted to at least get a near-final HFC script into the cloud!

Copy link
Member

@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.

Awesome, looks very close now! Feel free to implement my (hopefully) final suggestions or, if you're sick of iterating, I can quickly push 😄

Comment on lines +19 to +20
# The below channels in the test data do not have positions, set to bad
bads = ['G2-DS-Y', 'G2-DS-Z', 'G2-DT-Y', 'G2-DT-Z', 'G2-MW-Y', 'G2-MW-Z']
Copy link
Member

Choose a reason for hiding this comment

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

Great! This might be what you actually end up wanting to do in practice with your data, but for now it allows us to decide the right course of action. If it is something you'll do a lot, we can do something like mne.preprocessing.mark_invalid_position_bad(inst) that looks through all data channels (which would include MEG) and looks at ch['loc'] and marks ones as bad that don't have valid positions 👍

@georgeoneill
Copy link
Contributor Author

georgeoneill commented Apr 17, 2023

Hi @larsoner I think I agree with/understand most of the motivation. Happy for you to push those changes as I think you'll be able to get them to stick with a bit more precision than I can.

@larsoner
Copy link
Member

@georgeoneill sorry for the slow response here, I plan to tackle this tomorrow

@georgeoneill
Copy link
Contributor Author

No worries, no rush? Unless we need to get this in for the 1.4 deadline and I can have a go tonight?

@larsoner
Copy link
Member

It would be nice to have for 1.4 but we're going to push that release date/deadline at least a week I think so. I'll just adjust it on GitHub now -- we have a lot of PRs/issues marked that we need to sort through.

@georgeoneill
Copy link
Contributor Author

Okay, if you need me to step back in to make the revisions, let me know.

* upstream/main:
  BUG: Fix bug with paths (mne-tools#11639)
  MAINT: Report download time and size (mne-tools#11635)
  MRG: Allow retrieval of channel names via make_1020_channel_selections() (mne-tools#11632)
  Fix index name in to_data_frame()'s docstring (mne-tools#11457)
  MAINT: Use VTK prerelease wheels in pre jobs (mne-tools#11629)
  ENH: Allow gradient compensated data in maxwell_filter (mne-tools#10554)
  make test compatible with future pandas (mne-tools#11625)
@larsoner larsoner changed the title [WIP][ENH] Add support for Harmonic Field correction ENH: Add support for Harmonic Field correction Apr 21, 2023
@larsoner
Copy link
Member

Okay @georgeoneill I pushed some simplifying commits. Can you take a look at the diff now and see if you're happy with my changes? Hopefully you can see most (all?) places I changed simplified some stuff

@georgeoneill
Copy link
Contributor Author

Hi @larsoner, this looks excellent. I've just renamed one of the test functions. I think its ready to go!

@larsoner larsoner enabled auto-merge (squash) April 22, 2023 21:08
@larsoner
Copy link
Member

Okay marked for auto-merge when green, thanks for working through this one @georgeoneill !

@georgeoneill
Copy link
Contributor Author

Thank you for the assistance on this one.

@larsoner larsoner disabled auto-merge April 22, 2023 22:26
@larsoner larsoner merged commit 295b7c7 into mne-tools:main Apr 22, 2023
@georgeoneill georgeoneill deleted the hfc branch April 24, 2023 09:34
larsoner added a commit to larsoner/mne-python that referenced this pull request Apr 25, 2023
* upstream/main: (152 commits)
  FIX: missing channels/fiducials can be np.nan (mne-tools#11634)
  use py3.10 in precommit config (mne-tools#11648)
  MAINT: Unify GH Actions pytest (mne-tools#11644)
  MRG: Rename "Discourse" link in top navigation to "Forum" [ci skip] (mne-tools#11649)
  ENH: Add support for Harmonic Field correction (mne-tools#11536)
  Add pre-commit (mne-tools#11541)
  BUG: Fix bug with paths (mne-tools#11639)
  MAINT: Report download time and size (mne-tools#11635)
  MRG: Allow retrieval of channel names via make_1020_channel_selections() (mne-tools#11632)
  Fix index name in to_data_frame()'s docstring (mne-tools#11457)
  MAINT: Use VTK prerelease wheels in pre jobs (mne-tools#11629)
  ENH: Allow gradient compensated data in maxwell_filter (mne-tools#10554)
  make test compatible with future pandas (mne-tools#11625)
  Display SVG figures correctly in Report (mne-tools#11623)
  API: Port ieeg gui over to mne-gui-addons and add tfr gui example (mne-tools#11616)
  MAINT: Add token [ci skip] (mne-tools#11622)
  API: One cycle of backward compat (mne-tools#11621)
  MAINT: Use git rather than zipball (mne-tools#11620)
  ENH: Speed up code a bit (mne-tools#11614)
  [BUG, MRG] Don't modify info in place for transform points (mne-tools#11612)
  ...
larsoner added a commit to larsoner/mne-python that referenced this pull request Apr 25, 2023
* upstream/main: (117 commits)
  FIX: missing channels/fiducials can be np.nan (mne-tools#11634)
  use py3.10 in precommit config (mne-tools#11648)
  MAINT: Unify GH Actions pytest (mne-tools#11644)
  MRG: Rename "Discourse" link in top navigation to "Forum" [ci skip] (mne-tools#11649)
  ENH: Add support for Harmonic Field correction (mne-tools#11536)
  Add pre-commit (mne-tools#11541)
  BUG: Fix bug with paths (mne-tools#11639)
  MAINT: Report download time and size (mne-tools#11635)
  MRG: Allow retrieval of channel names via make_1020_channel_selections() (mne-tools#11632)
  Fix index name in to_data_frame()'s docstring (mne-tools#11457)
  MAINT: Use VTK prerelease wheels in pre jobs (mne-tools#11629)
  ENH: Allow gradient compensated data in maxwell_filter (mne-tools#10554)
  make test compatible with future pandas (mne-tools#11625)
  Display SVG figures correctly in Report (mne-tools#11623)
  API: Port ieeg gui over to mne-gui-addons and add tfr gui example (mne-tools#11616)
  MAINT: Add token [ci skip] (mne-tools#11622)
  API: One cycle of backward compat (mne-tools#11621)
  MAINT: Use git rather than zipball (mne-tools#11620)
  ENH: Speed up code a bit (mne-tools#11614)
  [BUG, MRG] Don't modify info in place for transform points (mne-tools#11612)
  ...
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.

ENH: Add offline homogeneous field correction for OPMs

4 participants