FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Replace 'normed' kwarg to hist by 'density' in tests and docs. by anntzer · Pull Request #10258 · matplotlib/matplotlib · GitHub

  1. matplotlib
  2. matplotlib

Repository navigation

Replace 'normed' kwarg to hist by 'density' in tests and docs. - #10258

Closed
anntzer wants to merge 1 commit into
matplotlib:masterfrom
anntzer:nonormed
ConversationCommits (1)ChecksFiles changed
Closed

Replace 'normed' kwarg to hist by 'density' in tests and docs.#10258
anntzer wants to merge 1 commit into
matplotlib:masterfrom
anntzer:nonormed

Conversation

anntzer commented Jan 16, 2018

Copy link
Copy Markdown
Contributor

PR Summary

handles #10200 (comment)

PR Checklist

  • Has Pytest style unit tests
  • Code is PEP 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

dstansby reviewed Jan 16, 2018
View reviewed changes
Comment thread lib/matplotlib/tests/test_axes.py
d2 = np.linspace(0, 10, 50)
fig = plt.figure()
ax = fig.add_subplot(111)
ax.hist((d1, d2), stacked=True, normed=True)

dstansby Jan 16, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I think this should stay as it is, because we should still be testing that normed works until it is removed.

efiring Jan 16, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

In that case, shouldn't the test have two forms, one for 'normed' and the other for 'density'? At a more general level, we never have tested everything, and never will--there are too many possible combinations. We need to concentrate on the most useful tests, putting our resources, and those of the CI services, where they will be most effective in improving mpl. Testing that a deprecated kwarg still works seems very low priority, doesn't it? The time of deprecation seems like a perfectly reasonable time to switch a test to the non-deprecated alternative.

dstansby Jan 16, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality
  • There is a test directly below this one for density.
  • Sure we've never tested everything, but we should aim to test everything. This test definitely isn't a CI bottleneck.
  • In my opinion, something being deprecated means it is still supported, but there are plans for it to be removed. If it's supported, it should be tested.

I'm not going to block on removing the normed test, but I am -2 on it.

efiring Jan 16, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Aiming to test everything doesn't make sense to me, for the reason stated above.

efiring approved these changes Jan 16, 2018
View reviewed changes

efiring left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I like it; but I acknowledge the disagreement of @dstansby.

anntzer commented Jan 16, 2018

Copy link
Copy Markdown
Contributor Author

At least the docs should be fixed, I basically don't care about what we do with the tests (but should suppress the warnings if we don't change them).

tacaswell commented Jan 17, 2018

Copy link
Copy Markdown
Member

I think @dstansby has a good point here. Can we keep the commit from #10200 for the test file (assuming tests pass, it looks like they are all failing over there) and everything else from this PR?

tacaswell added this to the v2.2 milestone Jan 17, 2018

anntzer commented Jan 17, 2018

Copy link
Copy Markdown
Contributor Author

Feel free to do your own mix and match... or I can rebase after #10200 is merged.

QuLogic commented Feb 10, 2018

Copy link
Copy Markdown
Member

Oh, didn't know about this. Since #10200 is merged and #10385 took care of the examples, I guess we don't need this PR.

QuLogic closed this Feb 10, 2018
QuLogic modified the milestones: v2.2, unassigned Feb 10, 2018
anntzer deleted the nonormed branch February 10, 2018 17:19
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

dstansby dstansby left review comments
efiring efiring approved these changes

Assignees

No one assigned

Labels

Documentation status: duplicate status: needs rebase topic: testing

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL