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.

Sign up for GitHub

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

Jump to bottom

annotate_clear -> clear_annotations #2569

Merged
merged 2 commits into from
Jun 22, 2020
Merged

annotate_clear -> clear_annotations #2569

merged 2 commits into from
Jun 22, 2020

Conversation

jcoughlin11
Copy link
Contributor

@jcoughlin11 jcoughlin11 commented Apr 29, 2020 •
edited
Loading

PR Summary

Makes the functionality of the annotate_clear method, well, clearer, by changing it to clear_annotations.

Addresses #2564

PR Checklist

  • Code passes flake8 checker
  • New features are documented, with docstrings and narrative docs
  • Adds a test for any bugs fixed. Adds tests for new features.

@jcoughlin11 jcoughlin11 mentioned this pull request Apr 29, 2020
Open
@munkm munkm added enhancement Making something better viz: 2D labels Apr 29, 2020
munkm
munkm approved these changes May 2, 2020
View reviewed changes
Copy link
Member

@munkm munkm left a comment

Choose a reason for hiding this comment

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

One minor comment that I'd like to see changed, but I'm happy to merge once that's updated!

yt/visualization/plot_window.py
Comment on lines 1109 to +1116
Clear callbacks from the plot. If index is not set, clear all
callbacks. If index is set, clear that index (ie 0 is the first one
created, 1 is the 2nd one created, -1 is the last one created, etc.)

.. note::

Deprecated in favor of `clear_annotations`.

Copy link
Member

Choose a reason for hiding this comment

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

I think the deprecation notice should be at the start of this docstring (it doesn't need to be in a note). If a user looks at this function, the very first thing they read should be that it has been deprecated in favor of clear_annotations.

@chummels
Copy link
Member

chummels commented May 3, 2020

Just a note about why I chose to make this originally called annotate_clear() when I implemented it. It was mostly because all of the existing annotations were annotate_x() or annotate_y(), so I wanted it to be visible and memorable for clearing them. But I'm fine on it being changed to clear_annotations().

@munkm
Copy link
Member

munkm commented May 5, 2020

I just noticed that #2562 is pointing to the 4.0 branch and this PR is pointing to master. This PR should also go to 4.0 or wait to be merged until after the 4.0 merge so it is accessible alongside the list_annotations feature.

@munkm munkm added the yt-4.0 feature targeted for the yt-4.0 release label Jun 11, 2020
@munkm munkm merged commit b6d9463 into yt-project:master Jun 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chummels chummels chummels approved these changes

@zingale zingale zingale approved these changes

@munkm munkm munkm approved these changes

@cphyc cphyc cphyc approved these changes

Assignees
No one assigned
Labels
enhancement Making something better viz: 2D yt-4.0 feature targeted for the yt-4.0 release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jcoughlin11 @chummels @munkm @cphyc @zingale