Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Cleanup psd example.#23426

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Closed
anntzer wants to merge1 commit intomatplotlib:mainfromanntzer:psd
Closed

Cleanup psd example.#23426

anntzer wants to merge1 commit intomatplotlib:mainfromanntzer:psd

Conversation

anntzer
Copy link
Contributor

  • Show all figures at once.
  • Group axes setter calls to have them take less room relative to the
    psd calls, which are the object of the example.
  • Align the noverlap examples to make the parallelism between them
    clearer.
  • Don't introduce a second random state with a single use.

PR Summary

PR Checklist

Tests and Styling

  • Has pytest style unit tests (andpytest passes).
  • IsFlake 8 compliant (installflake8-docstrings and runflake8 --docstring-convention=all).

Documentation

  • New features are documented, with examples if plot related.
  • New features have an entry indoc/users/next_whats_new/ (follow instructions in README.rst there).
  • API changes documented indoc/api/next_api_changes/ (follow instructions in README.rst there).
  • Documentation is sphinx and numpydoc compliant (the docs shouldbuild without error).

@@ -57,43 +55,35 @@
y = 10. * np.sin(2 * np.pi * 4 * t) + 5. * np.sin(2 * np.pi * 4.25 * t)
y = y + np.random.randn(*t.shape)

# Plot the raw time series
# Plot the raw time series.
fig = plt.figure(constrained_layout=True)
gs = gridspec.GridSpec(2, 3, figure=fig)
Copy link
Member

Choose a reason for hiding this comment

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

is there a point to using gridspec here/would mosaic maybe be better?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

I switched to the more idiomatic add_gridspec, but switching to subplot_mosaic would both result in slightly inconsistent layouts in this subexample relative to all others (unless you make the change in all subexamples) and 2) make the lines below no longer fit in 79 characters. I wouldn't be opposed if you make such a global change across all subexamples of the file, but let's not do this here?

@QuLogic
Copy link
Member

This seems good, but I thought we had a discussion about usingset overset_*, and I don't know where we landed. Maybe@timhoffm remembers?

- Show all figures at once.- Group axes setter calls to have them take less room relative to the  psd calls, which are the object of the example.- Align the noverlap examples to make the parallelism between them  clearer.- Don't introduce a second random state with a single use.
@jklymak
Copy link
Member

We are trying to deprecate these, so why are we fixing the example?

@anntzer
Copy link
ContributorAuthor

We've been talking about deprecating this for a long time and I don't think that'll happen any time soon (though I'd be happy to be proven wrong) so I think in the meantime fix improvements are still useful (unless we decide to "deprecate by removing from the docs", which may be a valid strategy too...).

story645 and timhoffm reacted with thumbs up emoji

@timhoffm
Copy link
Member

This seems good, but I thought we had a discussion about usingset overset_*, and I don't know where we landed. Maybe@timhoffm remembers?

The status is (#20631 (comment)):

  • set() andset_*() will both be supported, i.e. no deprecations
  • we need better documentation on the concepts - I just read in the above linke I volunteered volunteered to write this 👀
  • we have not decided, whether/where we want to useset() in the docs.

Because of the last point, I would currently not activately switch toset(). Nevertheless, In my eyes it's helpful to switch toset() in many examples where all theset_* are not a central aspect but only added to make the plot look better. - It's less distracting if they are all tucked away in a single call.

@jklymak
Copy link
Member

jklymak commentedJul 17, 2022•
edited
Loading

We've been talking about deprecating this for a long time and I don't think that'll happen any time soon (though I'd be happy to be proven wrong) so I think in the meantime fix improvements are still useful (unless we decide to "deprecate by removing from the docs", which may be a valid strategy too...).

There is a PR to deprecate it...#22920

@anntzer
Copy link
ContributorAuthor

Ah, sorry I missed that. Let's just not bother, then.

@anntzeranntzer deleted the psd branchJuly 18, 2022 08:56
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@story645story645story645 left review comments

Assignees
No one assigned
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

5 participants
@anntzer@QuLogic@jklymak@timhoffm@story645
[8]ページ先頭
©2009-2025 Movatter.jp