Open drammock opened 1 month ago
I agree that passing an empty list should be a no-up or raise an exception
the default could be changed to the "all" value you're proposing
the default could be changed to the "all" value you're proposing
Currently there is no default (it's a required kwarg), so nothing needs to change there. But since the behavior of channels=[]
would be changing, there would need to be a deprecation cycle.
+1 for the proposed change and a deprecation cycle.
Yesterday I did something like
mark_channels(bids_path=my_bp, channels=[], status="bad")
. I was expecting it to be a no-op (becausechannels
is an empty list) but in fact it marked all channels as bad. Now, the docstring is clear on this point, so I can't really complain about that. But I would argue that it is really unintuitive that passing a list of 1 channel name marks 1 channel, but a list of 0 channel names marks all channels (instead of zero channels).My use case: we have
prebad.txt
files listing which channels were noted as noisy during the recording. Sometimes those files are empty. It's useful to have that file exist and be empty, rather than just not existing at all, because then you know that someone looked for bad channels and found none (if "no bad channels" was indicated by absence of the file, you wouldn't know whether someone checked and found no bad channels, or just forgot to check at all).In that context, my script finds the
prebad
file, reads it, and if there are no channel names in that file, theprebads
variable (which I pass tomark_channels()
) is an empty list --- which does not do what I want it to!My suggestion is that if you want to have a signal value that will mark all channels as good/bad, that signal value should be something like
"all"
rather than[]
. If you agree, feel free to assign this issue to me and I can implement the change.