[BUG] - Issue with `shuffle_circular` about spiketools

[maestapereira]

Same bug found for shuffle_bins.
Cause for error: Have tested with multiple neurons and none of them worked. It seems the function does not work where there are more than one spikes per time unit (so, for example, one spike at 21.1 and another one at 21.8).
We have more than option on how to fix the bug (@TomDonoghue).

Suggestion:
Add one test, fix one bug, add another test to check if bug fixed.

1. (TEST1) Case where len(spike_train)<shuffle_min: error on np.random.randint (line 208):
How to test this:
- assert that shuffle_min is < len(spike_train)

2. (BUG) Size mismatch: shuffled_spikes[ind, :] = convert_train_to_times(temp_train) (line 208):
a) the allocated space for shuffled_spikes is ([n_shuffles, len(spikes)])
b) the space needed for spike_train is np.ceil(spikes[-1]).astype(int), which is necessarily <= len(spikes), once spike_train considers at most 1 spike per "time unit", whereas the original spikes (with len(spikes)) has more than 1 spike per "time unit".
c) the space needed for np.roll(spike_train, shuffle).shape (temp_train) is the same (np.ceil(spikes[-1]).astype(int)).
d) the sum(temp_train) <= (np.ceil(spikes[-1]).astype(int)), because not necessarily at every "unit" there is a spike.
e) so, when converting temp_train using convert_train_to_times(temp_train), the dimension we are getting is sum(temp_train), which <= (np.ceil(spikes[-1]).astype(int)) <= len(spikes).

3. (POTENTIAL FIXES FOR BUG)
a) don't use create_spike_train if we want the number of spikes to be conserved.
b) change create_spike_train to conserve number of spikes.
c) if we don't care about the conservation of the number of spikes, then we need to change the pre-allocated space from ([n_shuffles, len(spikes)]) to [n_shuffles, temp_train.sum()].

4. (TEST2) After bug is fixed:
Use array with at least one occurence of more than one spikes within the same "time unit". For example: np.array([2.52544554, 3.09250163, 3.88278307, 1996.76312233, 1998.5207464 , 1998.64321824])

[TomDonoghue]

To do for now (we might update / extend with #11 & #12).

PR1: Make a pull request updating some things for shuffle:

• document unit assumption of input spike times
• document relation between shuffle_min (in circular shuffle) and unit spike times

PR2:

• add a check to infer seconds or ms (for example, check if values go about 10000), and print warning if given seconds.
  • make this a separate function to check values (we can then recycle this function for use as needed).

[maestapereira]

PR1:

• Haven't pull-requested yet, but the modifications are in my mod_circular_shuffle_doc branch.
• Also noticed the doc in the create_spike_train function said seconds (it's supposed to be milliseconds, right?). So I changed that, it's in my mod_conversions_doc branch. Took the opportunity to update spike description in other functions to say milliseconds as well.

PR2:

• Wasn't sure where I should add the function to, so I created a new file inside utils (check_spikes_in_ms.py), within my branch mod_utils_check_spikes_in_ms. Haven't pull-requested yet.
• About the function check_spikes_in_ms itself:
  It checks two different things: (i) If there are two spikes within a same unit, and (ii) if the mean difference between spike times is less than 10 (an arbitrary number).
  Note: The seconds data I was working with had a mean difference between spike times of less than 1, but I wanted to be safe :)

[TomDonoghue]

Hey @maestapereira - I feel like I sorta lost track of this - did we address this by adding the checks for units (#14 - to be further conventionalized, see #11), and/or do you think there is something else to do here?

[maestapereira]

Hi @TomDonoghue - I think it's addressed! Only thing I would say is missing is calling the function in check.py where needed.

[TomDonoghue]

I think this is now fixed / addressed, at least in part by the updates related to time (#98)

@maestapereira - if you think there's anything else here, please feel free to re-open, but otherwise I'm going to close this for now.