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

Add title argument to plotting function #363

Open
wants to merge 1 commit into
base: plotting-ax
Choose a base branch
from

Conversation

chinandrew
Copy link
Collaborator

@chinandrew chinandrew commented Dec 5, 2020

Based off #361, so based for this PR should be switched to main and merged after it. Fixes #360.

Summary of changes:

  • Add an optional title argument to the plotting function.
  • Add test.
  • Add some docstrings in the test mocks that I thought might be useful.

@chinandrew chinandrew changed the title add title arg Add title argument to plotting function Dec 5, 2020
@capnrefsmmat
Copy link
Contributor

Hmm. Is there a way to set a subtitle? Because it seems like a good strategy would be to take a custom title and let the subtitle be date. Otherwise, if you set a title, the date doesn't get displayed at all, right?

@chinandrew
Copy link
Collaborator Author

Hmm. Is there a way to set a subtitle? Because it seems like a good strategy would be to take a custom title and let the subtitle be date. Otherwise, if you set a title, the date doesn't get displayed at all, right?

matplotlib has a suptitle() (yes with a p and not b) that places a title centered on the figure, but it doesn't look great

test

Alternatively you can add a \n and that will lead to a second line.

Is it a big problem if there's no date? A user always has the option to add the date, and if we force it in the subtitle and they don't want it, that could be annoying. The one case where I could see it making sense is when you pass in a df with multiple dates and it uses the most recent time value, having it tell you that date could be useful. However, we could also achieve that with a message/warning of some sort.

@capnrefsmmat
Copy link
Contributor

Ah, sorry, I was thinking of the animation method for some reason, maybe because it was being discussed on Slack. There it's important to have a date appear somewhere, because otherwise you have no idea which frame corresponds to what date.

@chinandrew
Copy link
Collaborator Author

Ah, sorry, I was thinking of the animation method for some reason, maybe because it was being discussed on Slack. There it's important to have a date appear somewhere, because otherwise you have no idea which frame corresponds to what date.

Good point. Maybe we add an optional add_date argument that adds f"\n{date}" to the end of a provided title?

@dshemetov
Copy link
Collaborator

This looks like a backwards-compatible change. Do we want to fix and merge or close? @capnrefsmmat

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants