annotate_clear is unclear

[munkm]

This is an issue I'm raising from #2498 and #2562 . In that PR a new function is added that lists the existing annotations added to a plot object. Following the convention of annotate_clear(), which clears the annotations on the plot, the author created a method called annotate_list(). I think in the case of these functions, we should modify them so that the action is the first word in the method, e.g. clear_annotations() and list_annotations(), since these are methods that operate on the plot annotations, rather than annotating a plot.

annotate_list() has been changed to list_annotations() by the author of that PR. annotate_clear() should be updated to match it. In this case, I think we'll want to maintain backwards compatibility with the old method name with a deprecation warning on annotate_clear, we'll want to update all uses of annotate_clear() in the docs to be clear_annotations(), and we'll want to add a link in the api docs to the updated function.

Does anybody have any issues with changing this?

[zingale]

I like this name change -- it seems more natural.

[jcoughlin11]

I like this change! I issued a small PR to address this (#2569)

[matthewturk]

What if: we also added an annotate_clearly that took as an argument another annotation, and then what it did was just do that other annotation, but with a much thicker linestyle? Like, "You can annotate velocity vectors, or you can annotate them very clearly like you'd used a sharpie"?

[munkm]

I think your suggestion would look amazing on a figure using the doom colormap, @matthewturk.

[neutrinoceros]

I agree that the new propropsed names would be clearer, but I do have a minor concern with it.
The current names allow the user to discover and learn about annotate_list and annotate_clear while playing with annotation callback through autocompletion, which I find is valuable. I think that's how I learned about them myself, I don't know if I would have liked the yt's annotation features as much is I missed those functions at first.

[munkm]

Hm, that's a good point @neutrinoceros. I wonder if there's a way we could make them discoverable without having ambiguous names? In the original PR I pointed out that with annotate_list the function really sounds like the user would be annotating a list to their plot, not listing the annotations of the plot. I can easily see a user make this mistake with the list function (though clear is a little more nuanced).

[matthewturk]

I think namespacing them would help, but is perhaps a bit too far. For instance, `p.annotate.vectors()` or something. But, I agree, this is a good point.

…

On Wed, Apr 29, 2020 at 3:47 PM Madicken Munk ***@***.***> wrote: Hm, that's a good point @neutrinoceros <https://github.com/neutrinoceros>. I wonder if there's a way we could make them discoverable without having ambiguous names? In the original PR I pointed out that with annotate_list the function really sounds like the user would be annotating a list to their plot, not listing the annotations of the plot. I can easily see a user make this mistake with the list function (though clear is a little more nuanced). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2564 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAVXO22LS3ULP3HMEVVXNDRPCG6NANCNFSM4MSBY74A> .

[munkm]

@matthewturk I think we should allow annotate_clearly to take a series of descriptive words so that the needed emphasis can be changed by the user. For example annotate_clearly('extra') should result in a different linethickness than annotate_clearly('humongous')

[neutrinoceros]

I think namespacing them would help, but is perhaps a bit too far.

I was thinking the same thing.
With a dedicated namespace we could write something like

p.annotate.sphere(...)
sphere = p.annotate.list_annotations()[0]
p.annotate.clarify(sphere)
p.annotate.clear_annotations()

[munkm]

I really like the namespacing idea and I think it would be more elegant to access the annotations. However, I'm concerned that namespacing all of our plot callbacks would be a major change for our users and would have to be included in a major release. Maybe we could set up the namespacing infrastructure just for operations on annotations (like clear and list). So annotations likeplot.annotate_grid() and plot.annotate_velocity() still would be accessed the same, but then these operations could be accessed with plot.annotations.clear() and plot.annotations.list()?

[neutrinoceros]

I second @munkm 's suggestion.
I would add, but this is a matter of personal tastes, that I'd prefer the namespace be named with the verb annotate rather than the noun annotations, though I'd be happy with both.

[munkm]

In this case clear and list are the verbs on the noun annotation though, right?

[neutrinoceros]

In this case clear and list are the verbs on the noun annotation though, right?

Indeed. As I said I have no strong opinion of what's best :-)

[munkm]

I think you bring up a good point @neutrinoceros and we need to think longer term about the namespace name!

for now I propose that we allow both #2562 and #2569 to be merged with this new naming convention. Once we decide on a good namespace name (either annotate or annotations) we can submit another PR to shift them there. That should be required before the next release.

[chummels]

As @neutrinoceros noted, when I created the annotate_clear() function, it was named so that it matched up with the existing annotations (e.g., annotate_velocity() and so it was discoverable. I'm fine changing it. But it may suffer that it won't be as easy for users to find. The namespace solution helps with that, but then this current proposed naming fix just seems like a temporary switch that will eventually be deprecated yet again in a short period. But I guess that's OK?

[neutrinoceros]

quick note that there's an old issue related to this one : #547