oist-ncbc / spykesim

Extended edit similarity measurement for high dimensional discrete-time series signal (e.g., multi-unit spike-train).
https://pypi.org/project/spykesim
MIT License
20 stars 3 forks source link

Barton Sternerg always returns first alignment for first 2 windows #25

Closed rcojocaru closed 3 years ago

rcojocaru commented 4 years ago

So, I had noticed that the returned profile always resembles closely one of the first two windows (in temporal order) in a cluster, so I checked the code.

barton_sternberg returns mat[i] for some reason instead of the final alignment, where i is 1 and is not affected by the for loop that follows. So mat[i] that is returned actually always corresponds to alignment1 between windows 1 and 2, so it is NOT representative for the entire cluster. I assume this is a simple coding mistake, but it should be corrected before it affects potential users.

def bartonsternberg(mats, sim_bp, niter): .......... i, j = 1, 2 # for test dp_max, dp_max_x, dp_max_y, bp, flip = sim_bp( mats[i].astype(np.double), mats[j].astype(np.double)) al1, al2 = clocal_exp_editsim_align_alt(bp, dp_max_x, dp_max_y, mats[i], mats[j], flip) mats[i] = al1 mats[j] = al2 al = (al1 + al2) / 2 processed[i] = True processed[j] = True .......... return mats[i]

KeitaW commented 4 years ago

Thanks for the good cache.

Could you try to substitute return mats[i] with return al to see if that'll fix the issue?

rcojocaru commented 4 years ago

Well, that would make more sense, to return al. Still, I think there are other problems with this subroutine.

For example, simvec is initialized before the first for loop, but I think it should be initialized inside, before the second (inner) for loop that looks for the window which is most similar to the current profile among the not processed windows. Otherwise you risk at some point to always return the same j (the same window) once a high similarity value is found, even if that window has been already processed. I assume that is not the intent.

Also, I don't really understand the purpose of the if clause inside the first for loop, it just seems to initialize processed in the first iteration, but processed was already initialized above.

I have some suggestions regarding changing this subroutine that I will send to you soon if you don't mind.

KeitaW commented 4 years ago

Sure! Feel free to post that in this space so that we can discuss it further. Thanks!

KeitaW commented 3 years ago

Feel free to reopen it for further discussion :)