search

jonescompneurolab / hnn-core

Simulation and optimization of neural circuits for MEG/EEG source estimates
https://jonescompneurolab.github.io/hnn-core/
BSD 3-Clause "New" or "Revised" License
55 stars 52 forks source link

[MRG] Make all plots with time end at tstop #683

Closed tianqi-cheng closed 6 months ago

tianqi-cheng commented 1 year ago

Continues the PR #604. Closes the issue #544.

ntolley commented 1 year ago

@rythorpe @jasmainak we decided to just cherry-pick from the old PR since it was paused so long ago (and we didn't have access to push to the PR directly)

tianqi-cheng commented 11 months ago

@rythorpe Hi Ryan, do you think I addressed the issues properly?

tianqi-cheng commented 11 months ago

This looks good @tianqi-cheng. The next steps will be to resolve my minor comments below and add a deprecation warning + updating whats_new.rst based on @jasmainak's recommendation.

One other issue I anticipate we'll need to address before merging is that CellResponse instances read in from spike files don't set the CellResponse.times attribute. Since the code below relies on this attribute always being defined, we'll need to find a fix for this.

Thanks! I will work on it.

katduecker commented 6 months ago

Hey, I have now taken on this pull request. I'm thinking about the options for the deprecation cycle of defining tmin and tmax for plot_dipole et al.

  1. Should it still be possible to set the xlims using tmax and tmin, and if tmax and tmin aren't defined then rasterplots and dipole are plotted from 0 to stop?
  2. OR should the data always be plotted from 0 to tstop, and the deprecation warning tells the user to set xlim after plotting? (i.e. setting tmin and tmax doesn't do anything) I'm happy to make all of these clear in the tutorials when I'm done!