-
Notifications
You must be signed in to change notification settings - Fork 52
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
[MRG] Make all plots with time end at tstop #683
Conversation
@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) |
@@ -467,16 +467,11 @@ def savgol_filter(self, h_freq): | |||
self.sfreq) | |||
return self | |||
|
|||
def plot(self, tmin=None, tmax=None, layer='agg', decim=None, ax=None, |
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 technically a backwards incompatible change. If some user has tmin
and tmax
in their script, it will stop working now. The correct fix is to add a deprecation cycle. But if we want to assume that it's unlikely many users have this in their scripts, we should mark it as a "Bugfix" in whats_new.rst
...
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 thought one reason to have tmin
was for burn in period @rythorpe may have an opinion ...
but it's true we can probably just set plt.xlim((tmin, tmax))
to achieve the same
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.
See my comment in the previous PR here. I don't think we need tmin
at the plotting level, however, it would be useful at the simulate_dipole
level for a burn-in period. That's a very different feature though and should probably be implemented in a separate PR.
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 agree that we should do a deprecation cycle for these input arguments though.
hnn_core/viz.py
Outdated
tmin : float or None | ||
Start time of plot in milliseconds. If None, plot entire simulation. | ||
tmax : float or None | ||
End time of plot in milliseconds. If None, plot entire simulation. |
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.
Why was this added? I agree that time bounds should be included somehow when generating a raster plot, but I believe we can just use cell_response.times
.
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.
@rythorpe for reference a lot of these were just copied from the previous PR. @tianqi-cheng you can remove tmin/tmax
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.
Ah okay. Since the diff is different here than in #604, I guess some of commits didn't get copied over. Perhaps we should cherry-pick the remaining commits so that we don't have to rehash some of our previous discussion points?
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.
We did cherry-pick, but PR had open comments that were not addressed. I just updated the codes and deleted the tmin and tmax and other related codes.
hnn_core/viz.py
Outdated
tmin : float | None | ||
Start time of plot in milliseconds. If None, plot entire simulation. | ||
tmax : float | None |
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.
Not sure about this either. Is it 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.
Also deleted them too.
Co-authored-by: Ryan Thorpe <[email protected]>
@rythorpe Hi Ryan, do you think I addressed the issues properly? |
@@ -1218,6 +1218,7 @@ def plot_laminar_csd(times, data, contact_labels, ax=None, colorbar=True, | |||
|
|||
ax.set_xlabel('Time (ms)') | |||
ax.set_ylabel('Electrode depth') | |||
ax.set_xlim(right=times[-1]) |
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 nitpick, but I'd recommend defining tstop
(or whatever you want to call it) before decimating so that you don't introduce a small error in the right x-limit.
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.
Hi Ryan, it seems that there is no decim in this function. Do you want me to do this for other functions which contain decim?
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.
Ah you're right. I didn't realize that _get_plot_data_trange
only crops the time series. Since this is the case, however, I don't think we need the added lines on L1207-1209 as they crop the time series to its original length (i.e., they don't actually crop anything off).
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.
Re: adding the decimate feature to plot_laminar_csd
, my vote would be to save it for a separate PR.
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 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. |
Co-authored-by: Ryan Thorpe <[email protected]>
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.
|
Continues the PR #604.
Closes the issue #544.