-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
MRG: Change Beer Lambert law default ppf value to 6 #9843
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
Conversation
|
FYI: @HanBnrd (also looking forward to your talk next week!) @rderollepot @kylemath @larsoner @agramfort ready for review, unless someone above disagrees |
|
Cancel that merge request, I will check the tutorials before requesting a merge. The scaling of results might now be different and I may need to change the tutorial text and ylims |
|
+1 for this change, but unfortunately we need to do it via deprecation :( This means in the signature you have and then to save us some pain, add an ignore line to |
Ah yes, of course. No worries, I will implement as you describe. Thanks for explaining how to do it in detail, that saved me a good 20 mins! |
Thank you Robert for your investigation on this point and for the clear sum up above ! |
|
@larsoner do we put in latest.inc items for future changes? Or should I just remove this from latest? |
|
Deprecations go in the API section |
|
@larsoner and @agramfort could you please review. The failing tests in one CI seem unrelated. Thanks |
Co-authored-by: Alexandre Gramfort <alexandre.gramfort@m4x.org>
This reverts commit 984e281.
This reverts commit 9f2b349.
|
Thanks @rob-luke ! |
|
|
||
| - Deprecate ``mne.viz.utils.center_cmap`` (:gh:`9851` by `Clemens Brunner`_) | ||
|
|
||
| - Add depreciation warning for the default partial pathlength factor of :func:`mne.preprocessing.nirs.beer_lambert_law` from 0.1 to 6 (:gh:`9843` by `Robert Luke`_) |
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.
Whoops, depreciation should be deprecation -- will fix in main
|
Thanks @agramfort and @larsoner for completing this PR |
Reference issue
The fNIRS processing uses the modified Beer Lambert law (BLL) to convert optical density data to changes in haemoglobin concentration. This process uses a scaling factor called the partial path length factor.
The largest fNIRS group that wrote the Homer software uses a ppf=6 value. Whereas we use a default value of 0.1. @larsoner already ran in to this issue before, and for his code needed to change the MNE function to use ppf=6 (#8711 (comment))
When I wrote the BLL code I had been using the default ppf value from the Huppert AnalyzIR toolbox (Santosa... Huppert 2018) of 0.1. Their approach was to use a partial volume correction (PVC=60) when determining the ppf. So their ppf = [dpf=6]/[pvc=60] = 0.1
I had assumed when writing our code that the 2018 paper was a better benchmark than the Homer 2009 paper (also by Huppert). However, I just had a phone call with Yucel (author of the fNIRS best practices paper), and they still recommend ppf=6. For many reasons, but one that stood out for me is that the pvc value is not well known and varies across the head and task.
What does this implement/fix?
To be in line with best practices and with the most prominent fNIRS software and match community expectations from existing literature I change our default ppf=0.1 to ppf=6
Additional information
Neither approach is right or wrong. But using ppf=6 is more inline with the community expectations and makes our results easier compared to the majority of the existing literature.
I've been going back and forth in my head about this for a while, but I think we should make the jump before the 0.24 release.
The tests already had a hard coded ppf value (didnt use the default), so they didnt require changing.