hnn-core icon indicating copy to clipboard operation
hnn-core copied to clipboard
verified publisher icon jonescompneurolab

Make all plots with time end at `tstop`

Open ntolley opened this issue 3 years ago • 12 comments

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.

ntolley avatar Aug 15 '22 16:08 ntolley

Does the cell_response object store tstop ?

jasmainak avatar Aug 15 '22 18:08 jasmainak

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

ntolley avatar Aug 15 '22 21:08 ntolley

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

rythorpe avatar Aug 16 '22 00:08 rythorpe

Can I work on this?

aritrasinha108 avatar Mar 01 '23 19:03 aritrasinha108

sure, what functions will be affected by this?

jasmainak avatar Mar 01 '23 20:03 jasmainak

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.

aritrasinha108 avatar Mar 01 '23 22:03 aritrasinha108

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

jasmainak avatar Mar 03 '23 23:03 jasmainak

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

aritrasinha108 avatar Mar 04 '23 15:03 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 avatar Mar 05 '23 20:03 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!

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

aritrasinha108 avatar Mar 06 '23 16:03 aritrasinha108

@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

ntolley avatar Oct 11 '23 21:10 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

Got it! I will take a look at it.

tianqi-cheng avatar Oct 12 '23 02:10 tianqi-cheng

Closed by #769

ntolley avatar Jul 31 '24 18:07 ntolley