Add plot_drift_raster_map widget #3068
Conversation
| peaks : np.array | ||
| The peaks array, with dtype ("sample_index", "channel_index", "amplitude", "segment_index") | ||
| peak_locations : np.array | ||
| The peak locations, with dtype ("x", "y") or ("x", "y", "z") |
There was a problem hiding this comment.
We may need more here for peak_locations just because the issue of what x y and z mean come up a lot on probe interface no?
There was a problem hiding this comment.
maybe just, as computed from the localize_peaks or spike_locations extension?
There was a problem hiding this comment.
That's fair so that the user knows that the input needs to be exactly from that function/extension.
There was a problem hiding this comment.
Another use for the glossary @zm711 !
|
Could we move this into the motion.py to centralize everything related in the palce ? |
| cmap="inferno", | ||
| clim=None, | ||
| alpha=1, | ||
| backend=None, |
There was a problem hiding this comment.
this backend argument must be None or matplotlib so it is undocumented. I think it is nicer to either document and state it's value is fixed, or (probably better?) remove as an option.
There was a problem hiding this comment.
it gets documented later, see widget_list
|
Ready for final review! @samuelgarcia @JoeZiminski @zm711 |
| self.figure, self.axes, self.ax = make_mpl_figure(**backend_kwargs) | ||
|
|
||
| if dp.times is None: | ||
| peak_times = dp.peaks["sample_index"] / dp.sampling_frequency |
There was a problem hiding this comment.
Sorry completely forgot, is this a place to use time_from_sample_index etc. to handle t_start and time vector?
There was a problem hiding this comment.
The problem here is that recording can be None (in that case sampling_frequency is required)
src/spikeinterface/widgets/motion.py
Outdated
| segment_index : int | None, default: None | ||
| If Motion is multi segment, the must be not None. | ||
| mode : "auto" | "line" | "map", default: "line" | ||
| How to plot map or lines. "auto" makes it automatic if the number of motion depths is too high. |
There was a problem hiding this comment.
Just thinking about this (but fine to say out of scope for this PR, but "map" vs "line" isn't the clearest right? I mean "line" makes more sense, but thinking from a new user's perspective would I know what "map" meant. What do you think @JoeZiminski?
There was a problem hiding this comment.
I kind of like map, but we can be clearer about it's definition. Agree that it probably deserves a different issue
There was a problem hiding this comment.
I meant should we define "map" in all our docstrings, not change the argument :)
| cmap : str, default: "inferno" | ||
| The colormap to use for the amplitude. | ||
| color : str, default: "Gray" | ||
| The color of the scatter points if color_amplitude is False. |
There was a problem hiding this comment.
something for us to discuss. We are super inconsistent with
color_amplitude vs 'color_amplitude' vs color_amplitude (with ticks, but they are being rendered so you can't see them in this third option....)
There was a problem hiding this comment.
Right. I like ticks because it's clear they refer to arguments and " for strings. We need another unification effort here
There was a problem hiding this comment.
I agree with you. I like ticks to emphasize arguments. I'll open an issue for the doc team :)
Co-authored-by: Zach McKenzie <[email protected]>
Co-authored-by: Joe Ziminski <[email protected]>
|
All done! thanks for the feedback :) Will merge after tests so I can update the hybrid tools |
Separate drift maps into a separate widget: