Skip to content

Rename boxplot's tick label parameter #27901

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

Merged
merged 7 commits into from
Mar 13, 2024
Merged

Conversation

saranti
Copy link
Contributor

@saranti saranti commented Mar 10, 2024

PR summary

Change the parameter name labels to tick_labels and deprecate the former name.

eg.
ax.boxplot(data, tick_labels=...)

Also update all the galley examples that used the parameter.

PR checklist

Copy link
Member

@timhoffm timhoffm left a comment

Choose a reason for hiding this comment

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

The exampls need to be updated to the new parameter name (see the GiHub Actions deprecation warnings in the "Files Changed" tab).

@github-actions github-actions bot added Documentation: examples files in galleries/examples and removed topic: ticks axis labels Documentation: API files in lib/ and doc/api labels Mar 11, 2024
@rcomer
Copy link
Member

rcomer commented Mar 11, 2024

There are also a couple of existing tests that are failing because they use the labels parameter (and warnings gets promoted to errors within our tests).

@saranti
Copy link
Contributor Author

saranti commented Mar 11, 2024

There are also a couple of existing tests that are failing because they use the labels parameter (and warnings gets promoted to errors within our tests).

Should I change those over on this PR?

@rcomer
Copy link
Member

rcomer commented Mar 11, 2024

Should I change those over on this PR?

Yes, the existing tests should use the new name I think.

Copy link
Member

@rcomer rcomer left a comment

Choose a reason for hiding this comment

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

A couple of questions which I’m not 100% sure about myself:

  • Should the key in the dictionaries that come out of cbook.bxpstats also be renamed?
  • Should the test be moved to a different file since it doesn’t check anything about legend any more?

@timhoffm
Copy link
Member

timhoffm commented Mar 12, 2024

A couple of questions which I’m not 100% sure about myself:

  • Should the key in the dictionaries that come out of cbook.bxpstats also be renamed?

I would leave that for now as the migration path is hard in terms of compatibility. We should do something like
#27788 for the return type of bxp. A migration becomes much easier with that. But that's for a separate/follow-up PR.

  • Should the test be moved to a different file since it doesn’t check anything about legend any more?

Yes, please move to test_axes.py.

Copy link
Member

@timhoffm timhoffm left a comment

Choose a reason for hiding this comment

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

This is good to go. Therefore, tagging as 3.9.

@timhoffm timhoffm added this to the v3.9.0 milestone Mar 13, 2024
@rcomer rcomer merged commit 4f56e1f into matplotlib:main Mar 13, 2024
@saranti saranti deleted the rename_tick branch March 13, 2024 14:57
timhoffm added a commit to timhoffm/matplotlib that referenced this pull request Mar 13, 2024
This is up for debate: I'm only +0.2 here.

The renaming was done in matplotlib#27901, which renamed the parameter `labels`
to `tick_labels` for `boxplot()` and `bxp()`.

One can take two views here:

- If `boxplot_stats()` is specifically for the input of `bxp()`, one
  can justify the renaming as being consistent with the `bxp()`
  signature. Note however, that the returned dict still
  contains the key "label" for back-compatibility. So that brings us
  an inconsistency between the parameter name and the returned dict key.

- One can alternatively view `boxplot_stats()` as a generic function to
  define box parameters. Here, we'd only have a general `label` for
  the boy and no information that this should be used as tick
  label.

If we could make a clean transition and also rename the dict key, I
would tend to go with the first view. But the inevitable inconsistency
of the fist view let's me sway towards the second view, so that with
the revert, we've effectively not touched `boxplot_stats()`.
timhoffm added a commit to timhoffm/matplotlib that referenced this pull request Mar 13, 2024
This is up for debate: I'm only +0.2 here.

The renaming was done in matplotlib#27901, which renamed the parameter `labels`
to `tick_labels` for `boxplot()` and `bxp()`.

One can take two views here:

- If `boxplot_stats()` is specifically for the input of `bxp()`, one
  can justify the renaming as being consistent with the `bxp()`
  signature. Note however, that the returned dict still
  contains the key "label" for back-compatibility. So that brings us
  an inconsistency between the parameter name and the returned dict key.

- One can alternatively view `boxplot_stats()` as a generic function to
  define box parameters. Here, we'd only have a general `label` for
  the boy and no information that this should be used as tick
  label.

If we could make a clean transition and also rename the dict key, I
would tend to go with the first view. But the inevitable inconsistency
of the fist view let's me sway towards the second view, so that with
the revert, we've effectively not touched `boxplot_stats()`.
timhoffm added a commit to timhoffm/matplotlib that referenced this pull request Mar 14, 2024
This is up for debate: I'm only +0.2 here.

The renaming was done in matplotlib#27901, which renamed the parameter `labels`
to `tick_labels` for `boxplot()` and `bxp()`.

One can take two views here:

- If `boxplot_stats()` is specifically for the input of `bxp()`, one
  can justify the renaming as being consistent with the `bxp()`
  signature. Note however, that the returned dict still
  contains the key "label" for back-compatibility. So that brings us
  an inconsistency between the parameter name and the returned dict key.

- One can alternatively view `boxplot_stats()` as a generic function to
  define box parameters. Here, we'd only have a general `label` for
  the boy and no information that this should be used as tick
  label.

If we could make a clean transition and also rename the dict key, I
would tend to go with the first view. But the inevitable inconsistency
of the fist view let's me sway towards the second view, so that with
the revert, we've effectively not touched `boxplot_stats()`.
timhoffm added a commit to timhoffm/matplotlib that referenced this pull request Mar 15, 2024
This is up for debate: I'm only +0.2 here.

The renaming was done in matplotlib#27901, which renamed the parameter `labels`
to `tick_labels` for `boxplot()` and `bxp()`.

One can take two views here:

- If `boxplot_stats()` is specifically for the input of `bxp()`, one
  can justify the renaming as being consistent with the `bxp()`
  signature. Note however, that the returned dict still
  contains the key "label" for back-compatibility. So that brings us
  an inconsistency between the parameter name and the returned dict key.

- One can alternatively view `boxplot_stats()` as a generic function to
  define box parameters. Here, we'd only have a general `label` for
  the boy and no information that this should be used as tick
  label.

If we could make a clean transition and also rename the dict key, I
would tend to go with the first view. But the inevitable inconsistency
of the fist view let's me sway towards the second view, so that with
the revert, we've effectively not touched `boxplot_stats()`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants