-
Notifications
You must be signed in to change notification settings - Fork 203
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add session displacement generation #3231
base: main
Are you sure you want to change the base?
Add session displacement generation #3231
Conversation
089cbc0
to
60c8e5e
Compare
72ca7fa
to
33254b9
Compare
33254b9
to
e996dee
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool! This seems really nice. I think my only question is whether there can be more logic sharing with the single-session generate_drifting_recording
, since some of the changes you've made seem like they could be helpful there. But, not sure how much sense that makes.
|
||
displacement_vectors = np.concatenate(displacement_vectors, axis=2) | ||
|
||
return displacement_vectors, displacement_unit_factor, displacement_sampling_frequency, displacements_steps | ||
|
||
|
||
def calculate_displacement_unit_factor( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe this could be called something like "simulate_linear_gradient_drift"? i was a bit confused reading it, but it seems to be generating drift which is 0 at the top of the probe and something not zero at the bottom?
maybe someone can help explain what exactly
displacement_unit_factor = factors * (1 - f) + f
ends up producing... is it like there is some global drift plus per-unit linear drift?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @cwindolf thanks a lot for this review. This function is a refactoring of this code. I like simulate_linear_gradient_drift
, for this PR I will keep the current naming for consistency with the old code. However I'll make an issue based on some of the points you raise in this PR (e.g. including some things in the within-session drift) and add a note on this there.
I agree I got quite confused the first (few) times looking through the use of non_rigid_gradient
. I think the easiest way to see it is with some example values. In the first part of the function, the dot-product of the displacement vector and the unit location (expressed as a vector from the probe origin, I am not sure where it is, maybe bottom-left). In the y-displacement only case, this is just the unit y position. These unit positions are scaled to [0, 1]
and called factors
.
The f
and expression you show ensure that the 'largest' unit location (e.g. near the top of the probe if the origin is bottom left) is scaled by 1 (no change). The scaling is linear across all unit positions, its kind of like a linspace
where the max value is 1 and f
sets the min value. e.g. looking at the smallest, largest and a middle location unit (i.e. factors 0, 1, and 0.5)
non_rigid_gradient=0.8
0 * 0.2 + 0.8= 0.8
0.5 * 0.2 + 0.8 = 0.9
1 * 0.8 + 0.2 = 1
non_rigid_gradient=0.2
0 * 0.8 + 0.2 = 0.2
0.5 * 0.8 + 0.2 = 0.6
1 * 0.8 + 0.2 = 1
So in the first case, the scaling of the units is only in the range [0.8, 1]
of the normalised position of the unit. But for the smaller non_rigid_gradient=0.2
, the scaling is between [0.2, 1]
.
I re-wrote the docstring of the function, let me know if its any clearer, I think there is still room for improvement, I am also not sure how much depth to go into.
) | ||
|
||
|
||
def _update_kwargs_for_extended_units( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a cool feature! in the single-session case, i have previously dealt with this by adding extra units that are off the probe to start. i'm wondering how this fits into the single session case... would this function be useful there too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cheers! The approach here is exactly the same, some additional simulated unit locations are generated that are off the probe. As the displacement is applied, these out-of-probe units are moved into the probe. I think this would also be useful in the single session case, as the probe drifts within a session more and more 'new' signal (i.e. signal that was not detected in the probes original position) will be introduced and could affect the correction.
I guess this is a difficult problem, as the nature of the 'new' units introduced into the region in which the probe is measuring will be random and presumably highly variable across preparations.
Hey @samuelgarcia @alejoe91 hope you both are good! I know things are very busy at the moment, but I was wondering if anyone might be available to give this a quick review? It would be useful for #3231 to merge this and it is (relatively) orthogonal from existing code. |
src/spikeinterface/generation/session_displacement_generator.py
Outdated
Show resolved
Hide resolved
src/spikeinterface/core/generate.py
Outdated
@@ -776,7 +786,7 @@ def synthesize_poisson_spike_vector( | |||
unit_indices = unit_indices[sort_indices] | |||
spike_frames = spike_frames[sort_indices] | |||
|
|||
return spike_frames, unit_indices | |||
return spike_frames, unit_indices, firing_rates |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure it's worth changing the output of this public function? Alternatively, you could add
firing_rates_array = _ensure_firing_rates(firing_rates, num_units, seed)
at line 154 and keep it as before.
I know @h-mayorquin uses this function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree on this one.
Plus, the firing rates are already passed to synthesize_poisson_spike_vector
, why return them? You can make an array of the input in one line if necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes great points that's a big oversight, thanks both!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this Joe!
src/spikeinterface/generation/session_displacement_generator.py
Outdated
Show resolved
Hide resolved
src/spikeinterface/generation/session_displacement_generator.py
Outdated
Show resolved
Hide resolved
dfe5ac9
to
8bceeaa
Compare
Co-authored-by: Chris Halcrow <[email protected]>
…iminski/spikeinterface into add_session_displacement_generation
Co-authored-by: Chris Halcrow <[email protected]>
…iminski/spikeinterface into add_session_displacement_generation
This PR adds an 'inter-session displacement' ground-truth recording generator. This is to act as test data for inter-session alignment e.g. #2626 #3126. The reason for this is to support inter-session alignment (e.g. #2626, #3126 ). The idea is to create separate recordings with the same templates but shifted unit locations across recordings. There are options to model:
recording_amplitude_scalings
allows scaling the injected template amplitudes to different sizes across recordings.This PR tries to use the existing motion machinery where possible, and makes some refactorings with this aim. The main changes are the refactorings, introduction of a new function for generating multi-session drifting recordings, and associated tests. Below are some sections to highlight usage, and example scripts for quick-running or deeper debugging.
I'm not sure about the name 'session displacement'. Maybe just 'generate_multi_session_recording()` is easier to understand. Also, please let me know if any other variable names are unclear.
Examples
Example 1
Set the amplitude of 2 units to zero between sessions, also with a 100 um shift. The units are shifted across sessions, and the top / bottom units are removed (e.g. simulating these neurons disappearing between sessions)_.
Example 2
Set
shift_units_outside_probe
toTrue
which introduces new units into the recording due to shift, alongside a 250 um shift. Note the top unit has been shifted out of the probe, the middle units are shifted up, and 2 new units are introduced at the bottom of the probe.Note
When running with a set of
n
num_units
, it is initially suprising that you do not always see exactlyn
units clearly (e.g. below, which was run withnum_units=5
. The main driver of this is that the defaultgenerate_unit_locations_kwargs
is between5
and45
. For low-amplitude neurons, a farz
axis will not generate enough signal to reach the probe (this is of course more realistic for simulation purposes). If you want to be sure you seen
units in the raster, setmargin_um
andmaximum_z
lower ingenerate_unit_locations_kwargs
e.g.:Quick Run Code
Full Debugging Code
TODO: