search

btel / SpikeSort

Spike sorting library implemented in Python/NumPy/PyTables
http://spike-sort.readthedocs.org
Other
27 stars 12 forks source link

fixed stacking of PCAs in fetPCA() #91

Closed espenhgn closed 11 years ago

btel commented 11 years ago

hi! thanks for the pull request. do you have a unit test for the function?

espenhgn commented 11 years ago

Hi Bartosz, I'm sorry, didn't write a test. Just encountered the problem that units would form well separated clusters on channels with insignificant unit amplitudes, from the way this np.vstack function were used.

Best, Espen

2013/9/12 Bartosz Telenczuk notifications@github.com

hi! thanks for the pull request. do you have a unit test for the function?

— Reply to this email directly or view it on GitHubhttps://github.com/btel/SpikeSort/pull/91#issuecomment-24334393 .

belevtsoff commented 11 years ago

@espenhgn thanks for spotting the problem.

The same holds for wavelet transforms. How about simply fixing the name labels instead of messing with data? For example, changing

names = ["Ch%d:PC%d" % (j, i) for i in range(ncomps) for j in
             range(n_channels)]

to 

names = ["Ch%d:PC%d" % (i, j) for i in range(n_channels) for j in
             range(ncomps)]

UPD: The tests don't pick up the problem because they are mostly single-channel

espenhgn commented 11 years ago

Well, I'm not strongly opinionated which way this should be ordered, as long as the labels are correctly assigned. If the latter ordering is more common, I'm not gonna argue against it.

Espen

2013/9/13 Dmytro notifications@github.com

@espenhgn https://github.com/espenhgn thanks for spotting the problem.

The same holds for wavelet transforms and maybe some other features I didn't check. How about simply fixing the names labels instead of messing with data? For example, changing

names = ["Ch%d:PC%d" % (j, i) for i in range(ncomps) for j in range(n_channels)]

to

names = ["Ch%d:PC%d" % (i, j) for i in range(n_channels) for j in range(ncomps)]

— Reply to this email directly or view it on GitHubhttps://github.com/btel/SpikeSort/pull/91#issuecomment-24390076 .

btel commented 11 years ago

@belevtsoff can you implement the change that you propose and write a unit test?

belevtsoff commented 11 years ago

@btel will do, just have to find time :)

btel commented 11 years ago

@belevtsoff any progress here?

belevtsoff commented 11 years ago

@btel It turned out to be non-trivial to update a pull-request from a remote fork. So, @espenhgn, could you grant me access to push to your fork? Otherwise, I'll have to move it to a separate branch and finish there, but in that case you'll loose credit for this pull request (which is bad, because our software is so extremely popular [notruth])

belevtsoff commented 11 years ago

@btel if there are no objections - I'm merging...