Expose hidden KiloSort1 parameters and descriptions #3956
Conversation
for more information, see https://pre-commit.ci
kushaangupta
changed the title
| ops["fshigh"] = params["freq_min"] # frequency for high pass filtering | ||
| ops["fslow"] = params["freq_max"] # frequency for low pass filtering (optional) | ||
| ops["ntbuff"] = params["ntbuff"] # samples of symmetrical buffer for whitening and spike detection | ||
| ops["scaleproc"] = 200.0 # int16 scaling of whitened data | ||
| ops["scaleproc"] = params["scaleproc"] # int16 scaling of whitened data |
There was a problem hiding this comment.
My only concern is that there are a few parameters that are always fixed (I guess someone could fork KS1 and change it themselves), but I think they always do the scaling at 200 for their own thing. So I will wait for @alejoe91 to comment on this since I started using KS at KS2 so maybe he knows better if we shouldn't expose certain parameters. Unless you really want to expose everything?
There was a problem hiding this comment.
Yeah maybe not everything, some parameters apparently aren't meant to be changed. I can remove those. It's hard for me to distinguish the internal parameters from the internally configurable ones. But some parameters that were earlier hardcoded like max/minFR, Th, lam etc were useful to be configured definitely
There was a problem hiding this comment.
I agree I think the ones you listed are fair to change. But I agree the documentation for the truly "internal" vs the configurable is not super clear. Let's wait for Alessio to comment as well and then we can come up with a strategy :)
There was a problem hiding this comment.
Hi guys! Yeah this looks good to me! :)
There was a problem hiding this comment.
@zm711 let me know if u need anything else for the merge
There was a problem hiding this comment.
I think this is in principle good to go. I just need to take a moment to carefully read through to make sure no parameters got missed. I'm away from computer tomorrow so I'll try to read it over Friday or Saturday to confirm that I don't see anything that didn't get transferred over.
| ops["nSkipCov"] = params["nSkipCov"] # compute whitening matrix from every N-th batch (1) | ||
| ops["whiteningRange"] = params[ | ||
| "whiteningRange" | ||
| ] # how many channels to whiten together (Inf for whole probe whitening, should be fine if Nchan<=32) |
There was a problem hiding this comment.
Based on this comment it seems like if we are allowing the user to set their own number then we should either prevent them from using > 32 or warn them. But we do say inf is possible.
Do you know if we should be doing some sort of check here?
There was a problem hiding this comment.
I'm not sure if I fully understand. Isn't it saying that it can be infinity if Nchan <=32?
There was a problem hiding this comment.
The issue is that users should use Nchan <=32, but alternatively they could whitened based on a whole probe by doing infinite, but this is not recommended. So the question would be do we
- assert <=32 to ensure that people use the more appropriate local whitening rather than allowing global whitening
- keep what you've done and let people do whatever they want, but default to 32
- let people do what they want, but warn if > 32
I'm note completely sure what the right answer is. I'm leaning toward 3, but if you have strong opinions I would love to hear arguments for 1 or 2.
There was a problem hiding this comment.
I updated it for 3
There was a problem hiding this comment.
Close. So what I would do instead is:
if params["whiteningRange"] > 32:
n_channels_whitening = params["whiteningRange"] if np.isfinite(params["whiteningRange"]) else "all"
warn(f"Kilosort recommends whitening with 32 or fewer channels you are whitening with {n_channels_whitening} channels"
ops["whiteningRange"] = params['whiteningRange"]But in this case at the top of the file you would need to add
from warnings import warnWant to give that a go instead? If not I can push this change myself.
There was a problem hiding this comment.
Now that testing is working again did you want to give a go editing to something like what I did above? Or would you like me to just push it myself?
Co-authored-by: Zach McKenzie <92116279+zm711@users.noreply.github.com>
|
Tests aren't working right now. Seems like pytest released a new version today. We are in the process of fixing the testing! |
Co-authored-by: Zach McKenzie <92116279+zm711@users.noreply.github.com>
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
Closes #3943