Skip to content
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

Remove legacy plot_psd* funcs/methods in our docs and codebase #11563

Merged
merged 18 commits into from Mar 20, 2023

Conversation

drammock
Copy link
Member

closes #11554

@larsoner WDYT about the changes to Report in 9b481b7? The kwargs used to be passed to plot_psd, now are passed to spectrum.plot(). Needs a deprecation cycle?

Comment on lines 2840 to 2844
fig = raw.plot_psd(fmax=fmax, show=False, **add_psd)
fig = raw.compute_psd(fmax=fmax, **add_psd).plot(show=False, )
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The kwargs used to be passed to plot_psd, now are passed to spectrum.plot(). Needs a deprecation cycle?

I'd say it's not worth it here given that the arguments are so similar. However, I do think you need to triage them, e.g., tmin/tmin/fmin/fmax need to go to compute_psd and plotting ones need to go to .plot. I don't think we want to remove any of these options, as people could reasonably want to just compute the PSD for the first 60 seconds for example, and say whether or not to plot in dB or whatever -- and these need to now go to two different methods.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we're going to triage then yeah, no need to deprecate. I'll pull out the triaging from the ToSpectrumMixin into a helper func so we can reuse it here.

@drammock drammock marked this pull request as ready for review March 17, 2023 21:59
@drammock
Copy link
Member Author

I've compared all the spectrum plots in the edited examples/tutorials against the corresponding plots in our dev docs, and they all look unchanged. This is ready for review.

Copy link
Member

@larsoner larsoner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one DRY idea but if it's not worth it or doesn't work easily feel free to merge

@drammock drammock enabled auto-merge (squash) March 20, 2023 10:23
@drammock drammock merged commit 2bd0c3e into mne-tools:main Mar 20, 2023
23 checks passed
larsoner added a commit to larsoner/mne-python that referenced this pull request Mar 20, 2023
* upstream/main:
  Fix bug in new `mne.sys_info()` (mne-tools#11577)
  Remove legacy plot_psd* funcs/methods in our docs and codebase (mne-tools#11563)
  The order of raw1.info["bads"] should not matter when concatenating with raw0.info["bads"] mne-tools#11501 (mne-tools#11502)
  MAINT: Fixes for matplotlib and pandas (mne-tools#11574)
  Fix Circle [circle deploy]
  Fix export to EDF format with a set physical range smaller than the data range (mne-tools#11569)
  Slightly rework `mne.sys_info()` (mne-tools#11568)
@drammock drammock deleted the remove-plot-psd branch March 21, 2023 13:36
@drammock drammock added the EOSS4 label Mar 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace .plot_psd() with .compute_psd().plot()
3 participants