lina-usc / pylossless

🧠 EEG Processing pipeline that annotates continuous data
https://pylossless.readthedocs.io/en/latest/
MIT License
24 stars 8 forks source link

`[epoch_ch_sd]``[init_method]` default value is `None`, which always results in 0 flagged epochs in `flag_ch_sd`. #12

Closed scott-huberty closed 1 year ago

scott-huberty commented 1 year ago

The default kwargs for epoch_ch_sd (which are both referenced in flag_ch_sd and passed into marks_array_2_flags as kwargs) are below. Note that the default value for init_method is null (which translates to None in python).

https://github.com/lina-usc/pylossless/blob/c444b0af2d7bb502e23d82d10dde095d540c71ae/assets/ll_default_config.yaml#L129-L135

In flag_ch_sd, if init_method is None, then an empty list is passed into flag_sd_t_inds, and nothing else is done. See line 501 in the code block below. Then see line 511, flag_sd_t_inds (which is an empty list in this case) is passed into self.flagged_epochs.add_flag_cat. This is why no bad_pylossless_ch_sd annotations are set at this step in the pipeline.

https://github.com/lina-usc/pylossless/blob/c444b0af2d7bb502e23d82d10dde095d540c71ae/pylossless/pipeline.py#L495-L511

IF you instead set init_method to q, for example, then you'll hit the code block starting in line 505 , and many flagged_epoch annotations will be created (I tested this on one file so far, and 24 bad_pylossless_ch_sd annotations were set).

3 questions:

  1. is setting [epoch_ch_sd][init_method] to null/None the default value we want for the default config?
  2. Do we even want to allow this value to be None?
  3. If the answer to 2 is Yes, should we at least provide a logger.warn to the end-user so they are aware that there will be no assessment for outlying epochs in flag_ch_sd? Because it took me an hour of code introspection to realize what was going on here.