Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Replace 'normed' kwarg to hist by 'density' in tests and docs.#10258
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| @@ -2932,7 +2932,7 @@ def test_hist_stacked_normed(): | |||
| d2 = np.linspace(0, 10, 50) | |||
| fig = plt.figure() | |||
| ax = fig.add_subplot(111) | |||
| ax.hist((d1, d2), stacked=True, normed=True) | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think this should stay as it is, because we should still be testing thatnormed works until it is removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
- There is a test directly below this one for
density. - Sure we've never tested everything, but we shouldaim 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 thenormed test, but I am -2 on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Aiming to test everything doesn't make sense to me, for the reason stated above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I like it; but I acknowledge the disagreement of@dstansby.
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). |
Feel free to do your own mix and match... or I can rebase after#10200 is merged. |
PR Summary
handles#10200 (comment)
PR Checklist