deprecate ImagePlotContainer.set_cbar_minorticks

[neutrinoceros]

The idea here is to uniformise the existing Colorbar api by using a single naming convention ('cbar' -> 'colorbar')

Considering the existing methods

• set_colorbar_label
• show_colorbar
• set_cbar_minorticks

I propose to just use "colorbar" everywhere with one change.
I'm keeping the old method for backwards compatibility but I also set a deprecation warning. I don't think this is common practice around here so it's unclear when the "deprecated" method should be removed (I'd suggest yt 4.0 to make it cleaner, but since I'm not sure we'll get a release before this one, it may be a bit of a rush).

[matthewturk]

THANK YOU for doing this. I think this is a fine change to make.

[neutrinoceros]

Also, I find it weird that it only works with state == "on", which is unusual in python and also unexpected in yt since most comparable methods use the plain boolean True to do just this, I'm proposing to accept both for backward compatibility (not sure if the "on" value should be deprecated or kept ad vitam)

[neutrinoceros]

Wait, I can actually NEVER make it do what it's supposed to do. Maybe it's a subtle mpl backend bug or something but now I'm worried this method may actually be plain broken.

[neutrinoceros]

@yt-fido test this please

[neutrinoceros]

There's something I'm not getting with the CI results. It seems to me that everything runs smoothly but it's still showing a "pending" status... Apparently this has to do with the build validation. Maybe caused by recent updates in #2448 , ping @munkm you might want to have a look at this :)

[munkm]

Hm based on the warnings I'm not sure it's from the changes in 2448, but I'll try to come up with a fix!

Other PRs have passed with this warning, so travis might just be acting a little weird too. All of the tests are passing, so with enough reviews this should be mergeable even with that pending icon.

[munkm]

ok #2451 fixes the config warnings.

[munkm]

I really like the idea of ensuring our colorbar functions are all consistently named. Thanks for updating this function!

I have a few comments:

• I do think the API should stay the same between the two functions. We also have a set_minorticks() method that uses the on and off strings, and it makes sense that these two functions should have the same functionality from a user's perspective. If you do want to change this function's API (to match the set_log() method and other methods), then the other one (and its docs) should be updated at the same time for consistency.
• Would you mind updating the documentation to use the new function doc/source/visualizing/plots.rst for the reference documentation on its usage)? That way users won't look at the docs and try to use the deprecated function.

[munkm]

Is there a reason you changed the API here from a string to a bool? I suspect a new user to yt would probably find it more intuitive to write 'on' or 'off' as an arg to set_colorbar_minorticks().

[munkm]

I think it would be ok to use the bool here as long as set_minorticks is also updated to be consistent.

[neutrinoceros]

I believe that most yt newcomers are at least familiar with matplotlib, which has a lot of such set_<stuff> methods and function arguments using plain boolean to decide whether a specific plot element should be visible or not, so I think it's a good opportunity to change the api to this behaviour.
I don't know which one of us has the most realistic idea of what feels intuitive to a new user, though I agree that the change should be consistently deployed to other methods.

[munkm]

I don't know which one of us has the most realistic idea of what feels intuitive to a new user

I don't really know how to feel about this, but I think both of us are trying to support our users in good faith.

[matthewturk]

OK, so I have to say, this is a fascinating discussion, especially when we look at the history of PlotWindow and the like. In some ways, what we've done is provide a big wrapper around a ton of matplotlib functionality to support a use case that I think is reasonably not-extensive, which is the idea that you have multiple fields in a single PlotWindow and you apply the same annotations, callbacks, etc, to all of them. But I think for the most part, people don't do that!

So we're in a weird place where so much of what we've done has been to support a use case that we don't really focus on much in the docs, even, and that includes a fair bit of wrapping of functionality. This isn't bad, and it does provide some nice ways to set defaults, but it's a layer between us and matplotlib.

Anyway, I think we should evaluate this, but I do think that any major API changes need to wait for 4.0, which shouldn't actually be too much longer to be master.

[neutrinoceros]

To add to this, I personally have mixed feelings about this use case. At the same time, it makes the simple, most repeated task extremely easy to perform, which is precisely what drove me into yt in the first place, but it also makes more refined plots somewhat harder to produce when one means to "talk to matplotlib" trough yt.

Anyway, I'm absolutely fine with delaying these changes to yt-4.0 if you think it's appropriate. However, I'm not sure I'm reading your statement right. Do you consider this a major change ? I guess not, but I'll admit I'm unsure.

[munkm]

I suspect these changes are not major API changes and can be merged before yt-4.0 (though Matt can also weigh in here if I've misinterpreted things). I think this is especially true because you've kept the old functions and have some depreciation warnings when they're called. You've certainly added to the usability of yt and I think this is a helpful contribution. 🙂

[matthewturk]

I agree!

[neutrinoceros]

Alright it should be more consistent now, though I think you referred twice the same documentation file by mistake, so there's must be another that need to be adressed.

Also, while browsing documentation I noticed to other methods using "cmap" in there names, so I'm going to deprecate them as well.

[neutrinoceros]

The one method I'm not willing to touch is ImagePlotContainer.set_cmap because it's very heavily used and would probably feel more like a disturbance to users.

[munkm]

Ok I edited the comment to be a little bit clearer. What I meant was that you should update the references with :meth:~yt.visualization.plot_window.AxisAlignedSlicePlot.set_cbar_minorticks to the updated function. They link back to the API docs that are autogenerated, so users will be redirected to the correct place. Then also update the examples that use set_cbar_minortics() to also use the new function. The references should be between lines 800 and 820 of that file.

[neutrinoceros]

Thanks for clarifying this. Indeed I missed some references in the .rst doc file. Should be fixed now !

[munkm]

Ok, I've looked over your most recent changes and left a few comments.

More generally, I think you should add a issue_deprecation_warning() for the deprecated functions. This function should be user-visible and is adapted from numpy.

[munkm]

I don't know which one of us has the most realistic idea of what feels intuitive to a new user

I don't really know how to feel about this, but I think both of us are trying to support our users in good faith.

[neutrinoceros]

Thanks for pointing out this function, I didn't know we had one ! I think I've addressed your concerns now :)

[Xarthisius]

@yt-fido test this please