BUG: Fix bug with optical_density #9522
Conversation
rob-luke
left a comment
rob-luke
left a comment
There was a problem hiding this comment.
I love that we are now getting down to the gritty details. That means things are broadly working.
Just a few questions. Mainly for my learning.
| min_ = min(min_, raw._data[pi].min() or min_) | ||
| # avoid == 0 | ||
| for pi in picks: | ||
| np.maximum(raw._data[pi], min_, out=raw._data[pi]) |
There was a problem hiding this comment.
Why do you set it to _min and not something like eps?
There was a problem hiding this comment.
eps is relative to 1. and is around 1e-12. Imagine you had data in the femto/1e-15 or smaller range, your np.finfo(float).eps would actually be a "large" value relative to this scale.
There was a problem hiding this comment.
Thanks @larsoner, turns out I didnt understand eps properly. I wonder how many bugs I have introduced over the years!
| c = np.corrcoef(filtered_data[ii], filtered_data[ii + 1])[0][1] | ||
| with np.errstate(invalid='ignore'): | ||
| c = np.corrcoef(filtered_data[ii], filtered_data[ii + 1])[0][1] | ||
| if not np.isfinite(c): # someone had std=0 |
There was a problem hiding this comment.
Does this mean that the channel is flat? I.e. it's all the same value? If this is the case would it not be better to throw the user a warning and tell them to set it to bad?
There was a problem hiding this comment.
Warning could be useful, yeah. This was mostly just laziness since I think/hope the outcome of the algorithm is still to clearly say the channel is bad anyway (e.g., with a SCI of 0). Maybe I should assert this in a test, then a warning isn't necessary? Or maybe we need both warning and sci=0?
There was a problem hiding this comment.
You're right, as long as it comes out with a 0 value the result will be the same to the user.
We should probably have a flat signal check as a seperate function check. I think I have seen that around somewhere in MNE already, Ill look.
There was a problem hiding this comment.
Yes we have a flat marking / annotating function
https://mne.tools/stable/generated/mne.preprocessing.annotate_flat.html
| sigma = 1.4826 * np.median(dev) | ||
|
|
||
| # Step 3d. Scale deviations by standard deviation and tuning parameter | ||
| if sigma == 0: |
There was a problem hiding this comment.
Does this also indicate a flat channel? As above.
There was a problem hiding this comment.
Do try and keep the code as close to the original as possible could we add the flat signal check as a block at the front?
There was a problem hiding this comment.
We could but I don't mind the small diff that's a clear fix here. It ends up being less clean to do it at the beginning I think because it creates redundant calculations
|
@rob-luke refactored a tiny bit and added tests, feel free to look again and merge if you're happy / convinced |
* upstream/main: MRG, ENH: Abstracted volume registration again (mne-tools#9521) MAINT: Avoid VTK 9.0.2 [skip actions] [skip circle] (mne-tools#9530) MRG: Test get_montage on fNIRS data (mne-tools#9524) FIX: fix topo plot when large channel distance gives rise to invalid values for arcsin (mne-tools#9528) use Polynomial.fit for poly fitting (mne-tools#9514)
|
Thanks @larsoner, I'm convinced 😉 |
2 participants
cc @rob-luke I think the protection against negative + zero values was not quite done safely enough