mne-tools / mne-hfo

Estimate/compute high-frequency oscillations (HFOs) from iEEG data that are BIDS and MNE compatible using a scikit-learn-style API.
http://mne.tools/mne-hfo/
BSD 3-Clause "New" or "Revised" License
13 stars 16 forks source link

[MRG] Initial implementation of Hilbert detector #23

Closed pmyers16 closed 3 years ago

pmyers16 commented 3 years ago

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:

pemyers27 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

adam2392 commented 3 years ago

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[bot] commented 3 years ago

Codecov Report

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

Impacted file tree graph

@@            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%) :arrow_down:
mne_hfo/detect.py 59.09% <38.88%> (+21.65%) :arrow_up:
mne_hfo/base.py 74.85% <54.90%> (+1.13%) :arrow_up:

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

Codecov Report

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

Impacted file tree graph

@@            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%) :arrow_up:
mne_hfo/utils.py 51.59% <46.51%> (-12.34%) :arrow_down:
mne_hfo/base.py 74.85% <54.90%> (+1.13%) :arrow_up:

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.