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

Define and document conventions on artist naming and keys for xyz_kwargs in plots module #66

Open
OriolAbril opened this issue Jul 6, 2024 · 2 comments

Comments

@OriolAbril
Copy link
Member

Functions in plots module have aes_kwargs, plot_kwargs, stats_kwargs whose keys we need to define. We should define how we want those to behave and document whatever naming conventions we are using to then be able to ensure consistency across plots. Otherwise we'll end up having too many plots and it will no longer be possible to use something intuitive-ish and consistent.

Things I think we have been using quite consistently:

  • Full names are preferred over abbreviations or acronyms
  • Each visual element has a name which is a valid key in plot_kwargs. This allows users to deactivate/skip any operation the plot does on.
  • Keys in aes_kwargs are keys in plot_kwargs. Some might only make sense for plot_kwargs and can be skipped (e.g. "remove_axis") and there can be some exceptions for different visual elements that share the same aesthetic mappings, but this should be done sparsely and only when the different visual elements end up calling the same backend function.
  • There aren't really rules or conventions on stats_kwargs so far. Also related to Allow precomputing #40

Open questions:

  • what should happen if there are invalid keys?

    I lean towards raising an error.

  • what should happen if there are valid keys but that will be ignored?

    I think here the best is probably to ignore them and go ahead, at most print a warning. This would allow for example: plot_dist(..., stats_kwargs={"kde": {"bw": 4}, "hist": {"bins": 20}}) to work and correctly process the kwargs with both kind="kde" and kind="hist" making the switch between different visualizations as painless as possible (as opposed to changing kind and modifying stats_kwargs).

  • Should we try to match the keys in stats_kwargs to the key in plot_kwargs where that data is more prominently encoded?

@OriolAbril
Copy link
Member Author

Should there be some kind of syntactic sugar to set multiple keys to the same value?

For example, to disable the ci, point estimate and title in plot_dist, something like:

plot_kwargs = {key: False for key in ("credible_interval", "title", "point_estimate")}

One option could be to allow "," or "+" in the keys, so we then internally "explode" those composed keys into the multiple ones. Then, setting ci and title to color red and disabling remove axis and point estimate would be something like:

plot_kwargs = {"title,credible_interval": {"color": "red"}, "point_estimate,remove_axis": False}

@OriolAbril
Copy link
Member Author

We had some discussion over a call, we think the main point that helps have an intuitive interface is having the same keys in both stats_kwargs and plot_kwargs whenever possible. Moreover, we prefer using a single key instead of multiple ones (e.g. single "density" key instead of "kde", "ecdf", "hist" depending on a kind parameter).

Regarding raising error, we should do that for invalid keys.

We'll also try the syntactic sugar mentioned above.

Implementation wise, there should be a helper function to take care of this as it will be common among all plots, something like proccess_kwargs(kwargs_dict: dict, valid_keys: Sequence) which returns a new dictionary. So it handles None->empty dict, dict->copy of dict and check keys + explode keys

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Planned
Development

No branches or pull requests

1 participant