[MRG] Initial implementation of Hilbert detector #23

pmyers16 · 2021-03-12T15:08:30Z

PR Description

Addresses part of: #18

Refactor detector into 3 step process of calculate, threshold, merge, and implement Hilbert Detector in that scheme.

Merge checklist

Maintainer, please confirm the following before merging:

pmyers16 · 2021-03-12T15:13:11Z

mne_hfo/utils.py

@@ -79,8 +84,144 @@ def _write_json(fname, dictionary, overwrite=False, verbose=False):
        print(os.linesep + f"Writing '{fname}'..." + os.linesep)
        print(json_output)

+def _band_zscore_detect(signal, sfreq, band_idx, l_freq, h_freq, n_times,


I did not quite fix the loop issue here, but I figured out how this works and tried to document it better.

pmyers16 · 2021-03-12T15:15:25Z

mne_hfo/utils.py

@@ -139,26 +282,166 @@ def compute_line_length(signal, win_size=6):
    return data[start:-stop]


-def threshold_std(signal, threshold):
+def compute_hilbert(signal, extra_params):


This one is a bit different because you compute the metric per freq band, so the return ends up being an ndarray

Can you document what the array axes mean, so I can take a closer look?

pmyers16 · 2021-03-12T15:17:33Z

mne_hfo/utils.py

+                    cycles_threshold, gap_threshold,
+                    zscore_threshold]
+            tdetects.append(_band_zscore_detect(args))
+


From here, we should have band indices, start and stop times, and max values during that band

pmyers16 · 2021-03-12T15:19:09Z

mne_hfo/utils.py

+    return tdetects
+
+
+def _run_detect_branch(detects, det_idx, HFO_outline):


I believe all this does is merge overlapping events

Yeah, let's see if we can refactor this then to use our version, which imo is more explicit.

This recursion imo is completely unnecessary and hard to decipher heh

pmyers16 · 2021-03-12T15:24:35Z

mne_hfo/utils.py

+    # corresponding to start and stop times
+    outlines = []
+    if len(tdetects):
+        while sum(tdetects[:, 0] != 0):


I need to look more into what this function's purpose is...I don't see how tdetects is changing, so not sure how this loop would ever end

pmyers16 · 2021-03-12T15:27:17Z

mne_hfo/utils.py

+        max_amplitude = max(outline[:, 3])
+        ch_hfos.append((start, stop, freq_min, freq_max,
+                        frq_at_max, max_amplitude))
+


This is where the additional info comes in. Hilbert calculates start and stop times, the frq band that the detection was found in, the max amplitude of the signal during this window, and the freq where that max amplitude occurred. Should we try to work this info into the output data structures?

So based on a threshold applied, the start/stop times can get converted to our hfo_event_arr_, while we can store in parallel a same-size array with freq-band (l_freq and h_freq) for each index, and the also another array with the same-size with the max_amplitude and also another array with the freq of the max amplitude.

We can either use these downstream, or scrap these and pipe them into a cohesive dataframe. This tbd, since I'm not sure how this would look, so better to be explicit rn.

Just to be clear, you are proposing that a HilbertDetector would have additional objects assigned to it at the end of class. We put start/stop times into the hfo_event_arr_ that we have been using. Then we have another array for freq bands and another array for amplitudes. Then we compare them just by matching row indices? Seems easy enough

Yep. These can all be "additional" data structures that are unique to just Hilbert detectors. We can later figure out how to best combine these if one would need programmatic access to it when doing post-hoc anallysis.

adam2392 · 2021-03-12T15:36:33Z

mne_hfo/detect.py

-                 verbose: bool = False):
+
+    def __init__(self,
+                 sfreq: float,


don't need.

adam2392 · 2021-03-12T15:40:23Z

mne_hfo/detect.py

+            X = mne.filter.filter_data(X, sfreq=self.sfreq,
+                                       l_freq=self.l_freq,
+                                       h_freq=self.h_freq,
+                                       method='iir', verbose=self.verbose)


can you remind me why iir vs fir for this one?

Do they use IIR in the original implementation?

If so, just document it here perhaps inline comment.

Yes, that is what was in the original implementation, so I can comment

pemyers27 · 2021-03-12T22:48:14Z

So right now, _post_process_ch_hfos simply calls the threshold_func to determine the threshold, then applies this threshold to the data to identify events. Obviously this is too simple for a Hilbert detector, or something more complex. My initial attempt seems slightly wrong, so what do you think about this workflow.
if threshold-method == '<method>': threshold_det_func = threshold_det_<method> event_identification_func = event_identification_<method> event_merging_func = event_merging_<method> extra_params = dict(values=values,...)
This will allow us to keep things simple for RMS/Line Length Detectors, but allow for any level of complexity at each step for the more complicated detectors.

adam2392 · 2021-03-15T02:15:51Z

So right now, _post_process_ch_hfos simply calls the threshold_func to determine the threshold, then applies this threshold to the data to identify events. Obviously this is too simple for a Hilbert detector, or something more complex. My initial attempt seems slightly wrong, so what do you think about this workflow.
if threshold-method == '<method>': threshold_det_func = threshold_det_<method> event_identification_func = event_identification_<method> event_merging_func = event_merging_<method> extra_params = dict(values=values,...)

Would you mind explaining to me with some more detail here? I'm not quite following. So Hilbert rn requires 3 thresholds: i) zscore of the Hilbert transform envelope, ii) number of cycles found and iii) gaps (idr what gaps were, so maybe we need to document that better).

The issue is _post_process_ch_hfos performs thresholding of only one metric. It seems based on our convos so far, that Hilbert needs to store the output of the thresholding step? Or how is the thresholding step connected to later steps that are not amenable to the current API of _compute_hfo_event, _post_process ?

Some thoughts: My impression is that Hilbert only needs to refactor the _band_z_score_detect step into what we call _post_process. It is essentially, taking the thresholds of the detector and looking for contiguously aligned windows that meet this "relatively complicated" threshold criterion. Thus, I thought all one needed to do was to rewrite fit, _compute_hfo_event, and _post_process_ch_hfos.

If thresholding needs to be a distinct step in of itself, then we can have each fit be: i) _compute_hfo_metrics, ii) threshold and iii) post_process into HFO detections. Would that work? You then can i) refactor the threshold step for all detectors out of the _post_proc function and then ii) refactor the _band_z_score_detect function into postprocess? This might make things more generalizable.

If I'm missing anything, lmk.

pmyers16 · 2021-03-17T19:30:14Z

So the basic idea of _run_detect_branch is to find the entire freq band that the HFO occurred in. Each freq band has a band_idx, so the goal is to check if there exists eventi[band_idx], eventj[band_idx+1] that overlap, and the recursion allows that to keep going for eventi[band_idx], eventj[band_idx+1], eventk[band_idx+2], .... is equal to one event

Once the overlapping events are merged, the HFO is defined as the [min start time, max end time] and the freq band is defined as [l_freq(band_idx), h_freq(band_idx+(n-1)] for n overlapping events.

Unfortunately I don't think computational efficiency is going to be good for this no matter what, but this is generally the new idea:
# For simplicity lets assume each detection has the form [start, stop, band_idx]
outlines = list
for detection in detections:
----if band_idx is 0:
--------append detection to outlines
----else:
--------for outline in outlines:
------------if detection overlaps outline:
----------------merge detection and outline
------------else:
----------------append detection to outlines

And the merging step would be something simple like:
def merge_outline(outline, detection):
----start = min(outline[0], detection[0]
----stop = max(outline[1], detection[1]
----freq_band = [outline[2][0], detection[2][1]]
----new outline = [start, stop, freq_band]

Then outlines should only have distinct HFO events

mne_hfo/base.py

adam2392 · 2021-03-21T01:08:46Z

mne_hfo/base.py

-            print(f'Using {threshold_method} to perform HFO '
-                  f'thresholding.')
+        if method == "time-windows":
+            return detections


we don't merge time-windows?

We do, but we do it upstream. I left a TODO where its done to refactor it here if you wanted. I think refactoring will slow it down a bit, which is why I held off

Hmm okay I will take a look

adam2392 · 2021-03-21T01:10:15Z

mne_hfo/detect.py

+        if band_method == 'log':
+            low_fc = float(filter_band[0])
+            high_fc = float(filter_band[1])
+            freq_cutoffs = np.logspace(0, np.log10(high_fc), n_bands)
+            self.freq_cutoffs = freq_cutoffs[(freq_cutoffs > low_fc) &
+                                             (freq_cutoffs < high_fc)]
+            self.freq_span = len(freq_cutoffs) - 1
+        elif band_method == 'linear':
+            self.freq_cutoffs = np.arange(filter_band[0], filter_band[1])
+            self.freq_span = (filter_band[1] - filter_band[0]) - 1


This looks good, but will not work because goes against sklearn-style API. Stick inside fit() at the beginning instead.

Meaning, sklearn imposes that one does not define additional things inside __init__ except what is passed in. Not sure why, but just something to do w/ it's API compliance.

Should I stick it in Hilbert's _compute_hfo_statistic function? That way we avoid a bunch of if statements in the base fit method

Sure that makes sense, unless there's any logic in fit that relies on this before calling _compute_hfo_statistic.

Nope, it just calls _check_input_raw, then right into _compute_hfo_statistic

adam2392

Some changes, but overall looks good, especially if unit tests are still passing.

Can you also add to the docstring of Detector, the general flow of an algorithm? This way, any downstream Detectors follow this abstraction.

E.g.

Step 1: compute a statistic on the data
Step 2: ...
etc.

Once you also add unit tests for Hilbert and validate that this works, then LGTM.

pmyers16 · 2021-03-30T15:25:36Z

mne_hfo/utils.py

+
+        # return the absolute value of the Hilbert transform.
+        # (i.e. the envelope)
+        hfx = np.abs(hilbert(signal))


This step is what was throwing the memory error

adam2392 · 2021-03-31T21:41:48Z

If you fix the CI and resolve the resolved comments, then I can take another review. Can merge and then we can move on with other high pri tasks.

codecov · 2021-04-01T20:33:17Z

Codecov Report

Merging #23 (5c95c39) into master (beb0932) will decrease coverage by 0.95%.
The diff coverage is 35.71%.

@@            Coverage Diff             @@
##           master      #23      +/-   ##
==========================================
- Coverage   70.96%   70.00%   -0.96%     
==========================================
  Files          13       13              
  Lines        1054     1167     +113     
==========================================
+ Hits          748      817      +69     
- Misses        306      350      +44

Impacted Files	Coverage Δ
mne_hfo/utils.py	`37.76% <26.35%> (-26.17%)`	⬇️
mne_hfo/detect.py	`59.09% <38.88%> (+21.65%)`	⬆️
mne_hfo/base.py	`74.85% <54.90%> (+1.13%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update beb0932...5c95c39. Read the comment docs.

adam2392 · 2021-04-01T20:55:23Z

Looks like _band_zscore_detect in utils.py isn't covered. I think you don't need that anymore right since you refactored things?

pmyers16 · 2021-04-01T21:07:26Z

Looks like _band_zscore_detect in utils.py isn't covered. I think you don't need that anymore right since you refactored things?

No, it is used by apply_hilbert function. I can test

codecov-io · 2021-04-06T14:17:36Z

Codecov Report

Merging #23 (f255b7e) into master (b9cc29c) will increase coverage by 1.26%.
The diff coverage is 46.03%.

@@            Coverage Diff             @@
##           master      #23      +/-   ##
==========================================
+ Coverage   70.96%   72.23%   +1.26%     
==========================================
  Files          13       13              
  Lines        1054     1167     +113     
==========================================
+ Hits          748      843      +95     
- Misses        306      324      +18

Impacted Files	Coverage Δ
mne_hfo/detect.py	`59.09% <38.88%> (+21.65%)`	⬆️
mne_hfo/utils.py	`51.59% <46.51%> (-12.34%)`	⬇️
mne_hfo/base.py	`74.85% <54.90%> (+1.13%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b9cc29c...f255b7e. Read the comment docs.

mne_hfo/base.py

initial implementation of Hilbert

9ed7d32

pmyers16 commented Mar 12, 2021

View reviewed changes

adam2392 reviewed Mar 12, 2021

View reviewed changes

mne_hfo/detect.py Outdated

verbose: bool = False):

def __init__(self,

sfreq: float,

Copy link

Member

adam2392 Mar 12, 2021

Choose a reason for hiding this comment

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

don't need.

adam2392 reviewed Mar 12, 2021

View reviewed changes

Merge branch 'master' into Hilbert

b79cefa

modify into three-step scheme

beeac09

adam2392 reviewed Mar 21, 2021

View reviewed changes

mne_hfo/base.py Outdated Show resolved Hide resolved

adam2392 reviewed Mar 21, 2021

View reviewed changes

mne_hfo/base.py Show resolved Hide resolved

adam2392 reviewed Mar 21, 2021

View reviewed changes

adam2392 requested changes Mar 21, 2021

View reviewed changes

pemyers27 and others added 10 commits March 25, 2021 11:48

move frq bands for hilbert

49e532f

broken tests

30c1425

add frq based detector func'

a534081

fix dimensions in hilbert detector

f9587ec

fix RMS and Linelength

245fcd5

run flake

c1ed904

fix flake

2ae3f41

run pydocstyle

8d1d79a

rerun pydocstring

36a1410

change test utils function names

20699a5

pmyers16 commented Mar 30, 2021

View reviewed changes

pmyers16 and others added 2 commits March 31, 2021 14:13

chunck the hilbert calculation

d592456

skip Hilbert test

55fa472

pemyers27 added 4 commits April 1, 2021 14:51

running flake

ffec966

run flake again

048fcec

add example to toctree

7df11cd

fix docstrings

5c95c39

pmyers16 and others added 2 commits April 5, 2021 21:11

add band detect test

97995d7

modify testing data

7905395

pmyers16 self-assigned this Apr 6, 2021

pmyers16 and others added 4 commits April 6, 2021 08:49

Merge branch 'master' into Hilbert

af78401

update whats_new

e11c6d7

fix checks

9c5dca0

modify whats_new

f255b7e

adam2392 reviewed Apr 6, 2021

View reviewed changes

mne_hfo/base.py Outdated Show resolved Hide resolved

adam2392 reviewed Apr 6, 2021

View reviewed changes

mne_hfo/base.py Outdated Show resolved Hide resolved

adam2392 reviewed Apr 6, 2021

View reviewed changes

mne_hfo/base.py Outdated Show resolved Hide resolved

adam2392 reviewed Apr 6, 2021

View reviewed changes

mne_hfo/base.py Outdated Show resolved Hide resolved

Apply suggestions from code review

4524bfe

adam2392 approved these changes Apr 6, 2021

View reviewed changes

adam2392 added 2 commits April 6, 2021 10:49

Merging readme.

85026fc

Merging.

5b88cda

adam2392 changed the title ~~[WIP] Initial implementation of Hilbert detector~~ [MRG] Initial implementation of Hilbert detector Apr 6, 2021

run flake

4f73dcf

adam2392 merged commit 91c8ed7 into master Apr 6, 2021

adam2392 deleted the Hilbert branch April 6, 2021 15:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MRG] Initial implementation of Hilbert detector #23

[MRG] Initial implementation of Hilbert detector #23

pmyers16 commented Mar 12, 2021 •

edited by adam2392

Loading

pmyers16 Mar 12, 2021

pmyers16 Mar 12, 2021

adam2392 Mar 12, 2021

pmyers16 Mar 12, 2021

pmyers16 Mar 12, 2021

adam2392 Mar 12, 2021

pmyers16 Mar 12, 2021

pmyers16 Mar 12, 2021

adam2392 Mar 12, 2021

pmyers16 Mar 12, 2021

adam2392 Mar 15, 2021

adam2392 Mar 12, 2021

adam2392 Mar 12, 2021

adam2392 Mar 12, 2021

pemyers27 Mar 12, 2021

pemyers27 commented Mar 12, 2021

adam2392 commented Mar 15, 2021 •

edited

Loading

pmyers16 commented Mar 17, 2021 •

edited

Loading

adam2392 Mar 21, 2021

pmyers16 Mar 25, 2021

adam2392 Mar 25, 2021

adam2392 Mar 21, 2021

adam2392 Mar 21, 2021

pmyers16 Mar 25, 2021

adam2392 Mar 25, 2021

pmyers16 Mar 25, 2021

adam2392 left a comment

pmyers16 Mar 30, 2021

adam2392 commented Mar 31, 2021

codecov bot commented Apr 1, 2021 •

edited

Loading

adam2392 commented Apr 1, 2021

pmyers16 commented Apr 1, 2021

codecov-io commented Apr 6, 2021

		return tdetects


		def _run_detect_branch(detects, det_idx, HFO_outline):

[MRG] Initial implementation of Hilbert detector #23

[MRG] Initial implementation of Hilbert detector #23

Conversation

pmyers16 commented Mar 12, 2021 • edited by adam2392 Loading

PR Description

Merge checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pemyers27 commented Mar 12, 2021

adam2392 commented Mar 15, 2021 • edited Loading

pmyers16 commented Mar 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adam2392 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adam2392 commented Mar 31, 2021

codecov bot commented Apr 1, 2021 • edited Loading

Codecov Report

adam2392 commented Apr 1, 2021

pmyers16 commented Apr 1, 2021

codecov-io commented Apr 6, 2021

Codecov Report

pmyers16 commented Mar 12, 2021 •

edited by adam2392

Loading

adam2392 commented Mar 15, 2021 •

edited

Loading

pmyers16 commented Mar 17, 2021 •

edited

Loading

codecov bot commented Apr 1, 2021 •

edited

Loading