8000 Better default behavior for boxplots when rcParams['lines.marker'] is set by pharshalp · Pull Request #15798 · matplotlib/matplotlib · GitHub
[go: up one dir, main page]
Skip to content

Better default behavior for boxplots when rcParams['lines.marker'] is set #15798

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Dec 30, 2019
Merged

Better default behavior for boxplots when rcParams['lines.marker'] is set #15798

merged 1 commit into from
Dec 30, 2019

Conversation

pharshalp
Copy link
Contributor
@pharshalp pharshalp commented Nov 30, 2019 •
edited
Loading

PR Summary

closes #15730
avoid using rcParams['lines.marker'] for boxplots.

Code snippet for reproducing the issue:

plt.rcParams['lines.marker'] = 's'
plt.boxplot(range(100))

Before this PR:
test

After this PR:
test1

PR Checklist

  • Has Pytest style unit tests
  • Code is Flake 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@pharshalp
Copy link
Contributor Author
pharshalp commented Nov 30, 2019 •
edited
Loading

should have run the tests locally... taking a closer look at what I missed... marking as WIP till I figure it out. fixed!

@pharshalp pharshalp changed the title closes gh-15730 WIP: closes gh-15730 Nov 30, 2019
@pharshalp pharshalp changed the title WIP: closes gh-15730 closes gh-15730 Nov 30, 2019
@pharshalp pharshalp changed the title closes gh-15730 closes gh-15730 (boxplot line markers) Dec 1, 2019
@pharshalp pharshalp changed the title closes gh-15730 (boxplot line markers) closes gh-15730 (a better default behavior for boxplots when rcParams['lines.marker'] is set) Dec 1, 2019
timhoffm
timhoffm reviewed Dec 2, 2019
View reviewed changes
Copy link
Member
@timhoffm timhoffm left a comment •
edited
Loading

Choose a reason for hiding this comment

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

The function is called for flierprops:

        final_flierprops = line_props_with_rcdefaults(
            'flierprops', flierprops)

I think we need 8000 a distinction that the marker is not overridden in that case. Could be passed as a parameter no_marker=True.

@tacaswell tacaswell added this to the v3.3.0 milestone Dec 5, 2019
tacaswell
tacaswell reviewed Dec 5, 2019
View reviewed changes
@tacaswell
Copy link
Member

To check my understanding, this is needed not because the marker is set by those methods, but because it is implicitly pulled in by one of the lower-level artists we use to make the box plot. By explicitly setting marker to '' we defeat that behavior?

This definitely needs a test given how distributed these issues are (needing to adjust an internal helper function to control the default behavior of classes someplace else in the code base).

Thanks for your work @pharshalp .

@pharshalp
Copy link
Contributor Author
pharshalp commented Dec 5, 2019 via email

@jklymak jklymak changed the title closes gh-15730 (a better default behavior for boxplots when rcParams['lines.marker'] is set) Better default behavior for boxplots when rcParams['lines.marker'] is set Dec 5, 2019
@pharshalp
Copy link
Contributor Author

@timhoffm implemented your suggestion of adding a parameter no_marker=True.
@tacaswell added a test (without relying on image comparison) to test that the marker is overridden by '' only when inheriting it from plt.rcParams['lines.marker'] and this override mechanism doesn't affect markers specifically set for boxplots using plt.rcParams['boxplot.xxxx'].

timhoffm
timhoffm reviewed Dec 22, 2019
View reviewed changes
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.

Essentially good to go. I'd just invert the logic of the marker flag.

timhoffm
timhoffm approved these changes Dec 24, 2019
View reviewed changes
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.

Should be squashed before merging.

@pharshalp
Copy link
Contributor Author
pharshalp commented Dec 24, 2019 •
edited
Loading

I think I messed up while squashing commits. Please bear with me as I untangle the mess :D.

pheww... done! (Out of habit, I had merged the current master into my branch. This introduced irrelevant changes to this PR. All ok now!).

jklymak
jklymak approved these changes Dec 24, 2019
View reviewed changes
Copy link
Member
@jklymak jklymak left a comment

Choose a reason for hiding this comment

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

Do you test the kwarg?

pharshalp
pharshalp commented Dec 24, 2019
View reviewed changes
lib/matplotlib/tests/test_axes.py
test_data = np.arange(100)
test_data[-1] = 150 # a flier point
bxp_handle = ax.boxplot(test_data, showmeans=True)
for bxp_lines in ['whiskers', 'caps', 'boxes', 'medians']:
Copy link
Contributor Author
@pharshalp pharshalp Dec 24, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Do you test the kwarg?

@jklymak yes, it gets checked (for 'whiskers', 'caps', 'boxes', 'medians') when the line markers are forced to '' irrespective of what the rcParams['lines.marker'] says (explicitly set to 's' in this test). [This implicitly tests use_marker=False].

    for bxp_lines in ['whiskers', 'caps', 'boxes', 'medians']:
        for each_line in bxp_handle[bxp_lines]:
            # Ensure that the rcParams['lines.marker'] is overridden by ''
            assert each_line.get_marker() == ''

I am also checking that the markers are allowed for fliers and means (and not forced to ''). [This implicitly tests use_marker=True].

    # Ensure that markers for fliers and means aren't overridden with ''
    assert bxp_handle['fliers'][0].get_marker() == 'o'
    assert bxp_handle['means'][0].get_marker() == '^'

@pharshalp pharshalp requested a review from tacaswell December 28, 2019 20:24
@timhoffm timhoffm merged commit 29a1afe into matplotlib:master Dec 30, 2019
@timhoffm
Copy link
Member

Thanks for the contribution!

@pharshalp
Copy link
Contributor Author

Thanks all for the helpful review!

@pharshalp pharshalp deleted the boxplots_line_markers branch January 27, 2020 17:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jklymak jklymak jklymak approved these changes

@timhoffm timhoffm timhoffm approved these changes

@tacaswell tacaswell Awaiting requested review from tacaswell

Assignees
No one assigned
Labels
topic: rcparams
Projects
None yet
Milestone
v3.3.0
Development

Successfully merging this pull request may close these issues.

Setting lines.marker = s in matplotlibrc also sets markers in boxplots
4 participants
@pharshalp @tacaswell @timhoffm @jklymak
0