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 an annotate_list method #2498

Closed
wants to merge 1 commit into from
Closed

add an annotate_list method #2498

wants to merge 1 commit into from

Conversation

zingale
Copy link
Member

@zingale zingale commented Mar 24, 2020

PR Summary

The annotate_clear() method takes an optional index of the annotation to remove, but there
is not high-level method to see the indices of the currently applied annotations. This PR adds
an annotate_list() method that simply prints the annotations and their indices.

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.

@zingale
Copy link
Member Author

zingale commented Mar 24, 2020

Note: I also fixed a sphinx error in the annotate_clear title ~ length.

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.

Since you've added a new callback, can you add it to the api reference for the list of callbacks at doc/source/reference/api/api.rst too, so that the api reference is generated too?

Thanks for this feature! I think it'll be really useful!

@@ -1119,6 +1119,15 @@ def annotate_clear(self, index=None):
self.setup_callbacks()
return self

def annotate_list(self):
Copy link
Member

Choose a reason for hiding this comment

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

Ok, I see that you're matching the annotate_clear method in the way that this operates on all annotations. That said, I'm wondering if it's maybe clearer to name it something like list_annotations? I feel like that might be more intuitive when looking through different operations one could do on a plot. If I were casually using yt, I'd think that annotate_list would be a specific annotation of a list object, just like annotate_scale annotates a scale.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree, it would be clearer, and I'm happy to change the name, but I was trying to match the convention already in place. E.g., clear_annotations() would also be more clear than annotations_clear()

@munkm munkm added the new feature Something fun and new! label Apr 24, 2020
@zingale
Copy link
Member Author

zingale commented Apr 26, 2020

because of some fork problems on my end, I will close and reissue this PR. Sorry :(

@zingale zingale closed this Apr 26, 2020
@zingale zingale mentioned this pull request Apr 26, 2020
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature Something fun and new!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants