Make all plots with time end at tstop

[ntolley]

Currently some of the plots like cell_response.plot_spikes_raster() do not end at tstop. This change will be useful for plotting functionality in the dashboard gui.

[jasmainak]

Does the cell_response object store tstop ?

[ntolley]

It has the times attribute so we can just infer tstop from that

[rythorpe]

Is there a reason we didn't implement this before? (I feel like this came up in discussion at one point....)

[aritrasinha108]

Can I work on this?

[jasmainak]

sure, what functions will be affected by this?

[aritrasinha108]

I looked into all the plots that are defined inside hnn_core.viz. Among all the plots that plotted against time, there are three functions that are needed to be changed. they are:

1. hnn_core.viz.plot_spike_raster
2. hnn_core.viz.plot_connectivity_matrix
3. hnn_core.viz.plot_laminar_csd

The other plots like plot_dipole, plot_laminar_lfp, plot_tfr_morlet, etc. have added time limits (if necessary) to their plots (both upper and lower) as tmin and tmax. I plan to follow a similar procedure so that all the other functions which are using the plot functions remain unchanged.

[jasmainak]

why would plot_connectivity_matrix have a tstop? Did you read the documentation related to this function?

[aritrasinha108]

Sorry, My bad. Only hnn_core.viz.plot_spike_raster and hnn_core.viz.plot_laminar_csd are required to have a tstop or a tmax. I have added a PR for this. Could you please look into it and see if I am in the right direction. Thanks

[rythorpe]

Sorry, My bad. Only hnn_core.viz.plot_spike_raster and hnn_core.viz.plot_laminar_csd are required to have a tstop or a tmax. I have added a PR for this. Could you please look into it and see if I am in the right direction. Thanks

I think you'll also need to add tmax to viz.plot_spikes_hist() - other than that lgtm!

[aritrasinha108]

Sorry, My bad. Only hnn_core.viz.plot_spike_raster and hnn_core.viz.plot_laminar_csd are required to have a tstop or a tmax. I have added a PR for this. Could you please look into it and see if I am in the right direction. Thanks

I think you'll also need to add tmax to viz.plot_spikes_hist() - other than that lgtm!

@rythorpe thanks a lot. I have added the parameters and refactored it accordingly for spikes_hist too in the PR.

[ntolley]

@tianqi-cheng can you take a look at this as your next issue? If you read the discussion you'll see that there is an existing PR (#604 ) that is almost ready to be merged

Go ahead and read over the code/suggestions. Eventually you'll need to fetch the branch for that PR to your local computer so that you can push back changes to the existing PR: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally

[tianqi-cheng]

@tianqi-cheng can you take a look at this as your next issue? If you read the discussion you'll see that there is an existing PR (#604 ) that is almost ready to be merged

Go ahead and read over the code/suggestions. Eventually you'll need to fetch the branch for that PR to your local computer so that you can push back changes to the existing PR: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally

Got it! I will take a look at it.