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

ENH: add support for passing arbitrary kwargs for axes.contours in annotate_contour #4460

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

neutrinoceros
Copy link
Member

@neutrinoceros neutrinoceros commented Jun 2, 2023

PR Summary

incidentally fix #4459

@neutrinoceros neutrinoceros added bug api-consistency naming conventions, code deduplication, informative error messages, code smells... viz: 2D deprecation deprecate features or remove deprecated ones labels Jun 2, 2023
@neutrinoceros neutrinoceros added this to the 4.3.0 milestone Jun 2, 2023
@neutrinoceros neutrinoceros marked this pull request as ready for review June 3, 2023 22:14
@neutrinoceros neutrinoceros force-pushed the no_mpl_rc_runtime_mod branch 3 times, most recently from f8b9cdc to 8524123 Compare June 4, 2023 09:08
@neutrinoceros neutrinoceros marked this pull request as draft June 4, 2023 09:14
@neutrinoceros
Copy link
Member Author

I'm going to prioritise #4475 over this.

@neutrinoceros neutrinoceros changed the title ENH: add support for passing arbitrary kwargs for axes.contours in annotate_contour ENH: add support for passing arbitrary kwargs for axes.contours in annotate_contour (⏰ wait for #4475) Jun 8, 2023
@neutrinoceros neutrinoceros changed the title ENH: add support for passing arbitrary kwargs for axes.contours in annotate_contour (⏰ wait for #4475) ENH: add support for passing arbitrary kwargs for axes.contours in annotate_contour Jun 9, 2023
@neutrinoceros neutrinoceros marked this pull request as ready for review June 9, 2023 19:17
@neutrinoceros
Copy link
Member Author

rebased to resolve merge conflicts.
To be explicit, this is ready for review :)

Copy link
Member

@brittonsmith brittonsmith left a comment

Choose a reason for hiding this comment

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

Is there an advantage to replacing the plot_args dict with **kwargs? My impression is that we had moved away from **kwargs toward dicts as a means of keeping keywords grouped according to destination. For example, in this same function, we have a text_args dict. Unless there is a specific benefit, I'm not sure I support this change.

yt/visualization/plot_modifications.py Show resolved Hide resolved
@neutrinoceros
Copy link
Member Author

My impression is that we had moved away from **kwargs toward dicts as a means of keeping keywords grouped according to destination. For example, in this same function, we have a text_args dict. Unless there is a specific benefit, I'm not sure I support this change.

This strategy still has value but it's not necessary to have all arguments be passed through dictionaries: (n-1) dictionaries arguments (with n, the number of destinations) is sufficient ! My point (and proposed change) is that we can still leverage **kwargs (which is very idiomatic in matplotlib) for the most common destination without limiting design space.

@brittonsmith
Copy link
Member

The minor benefit I see from keeping text_args and plot_args is that it is clear where the keywords are being shipped off to. Other than this, this change feels like mostly a lateral move with the downside of breaking backward compatibility. I think this is the point where it is probably worth getting other opinions. I'd rather not see API changes made or blocked based on individual opinions, but I am happy to defer to a majority. @matthewturk and I have not discussed this (although maybe you have with him), so he should have a fresh opinion.

@neutrinoceros
Copy link
Member Author

The way I see it there are three options to choose from:

  1. keep the API from main (no support for **kwargs)
  2. support **kwargs and deprecate plot_args (this is the current state of the branch)
  3. support both APIs (same as this branch, but without the deprecation)

I don't think any option is ideal: there are precedents to support options 1. and 2., and I have a personal preference to not keep support for more than one API for ever, but maybe option 3. is probably an agreeable middle ground ?

@neutrinoceros neutrinoceros removed this from the 4.3.0 milestone Sep 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-consistency naming conventions, code deduplication, informative error messages, code smells... bug deprecation deprecate features or remove deprecated ones viz: 2D
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: yt may leak matplotlib configuration globally at runtime
2 participants