DEP: Deprecate random_integers #6931
Conversation
|
Should raise this on the list. Would be good if @rkern weighs in there. |
|
Sent out email here |
|
☔ The latest upstream changes (presumably #6936) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Is this good to merge? Travis and Appevoyor are both happy, the branch no longer has any merge conflicts, and judging from the email, @rkern has given the green light. |
| @@ -255,6 +255,18 @@ def test_random_integers_max_int(self): | |||
| desired = np.iinfo('l').max | |||
| np.testing.assert_equal(actual, desired) | |||
|
|
|||
| def test_random_integers_deprecated(self): | |||
There was a problem hiding this comment.
Because Deprecation warnings are error for development testing and only raised once from a giving location by default, some care needs to be taken here. There is a _DeprecationTestCase in numpy/core/tests/test_deprecations.py that can be used as a base class and perhaps should be moved into numpy.testing, another, somewhat simpler option is
with warnings.catch_warnings():
warnings.simplefilter("error", DeprecationWarning)
assert_raises(DeprecationWarning, ...)
@seberg Thoughts about moving the _DeprecationTestCase?
There was a problem hiding this comment.
@charris : Could you clarify your comment? There seems to be a mixture of suggestions for this actual PR and suggestions of a greater infrastructural change. Should I move my test into test_deprecations.py as a derived class of _DeprecationTestCase?
There was a problem hiding this comment.
Yep, one is a more invasive change than the other. Quick and dirty is the simple option, the move is somewhat more complicated, but maybe provides functionality other spots would like. The idea isn't to move these tests, but rather put _DeprecationTestCase, without the underscore, someplace where you can get at it. Let's wait a bit and see if we get a second opinion.
There was a problem hiding this comment.
I'm -0 on adding _DeprecationTestCase to the public API, because it feels a bit underbaked (assert_deprecated's calling convention is super complicated!), so probably we shouldn't set it in stone as-is.
No objections to refactoring our test suite to have more useful internal tools though. Or maybe assert_warns should be clever enough to set up the warning context properly, if it isn't already.
There was a problem hiding this comment.
Sounds like quick and dirty for now.
There was a problem hiding this comment.
@charris : "quick and dirty" = move test into test_deprecations.py or that with warnings.catch_warnings() block? Both seem to fit the criterion.
There was a problem hiding this comment.
Q&D would be least work, e.g., with the catch_warnings environment.
There was a problem hiding this comment.
We have this new thing that clears the warning registry? It might go far enough. It is definetely half backed, it was backed by me getting desperate with avoid warning registry corruption.
If you do an assert_warns with "always" in the catch_warnings context, and then an assert_raises when putting DeprecationWarning to "error" you have it I think. I think we also have some new stuff (forgot where), which attempts to clear the warning registry.
There was a problem hiding this comment.
Yep, clearing the registry is a good safety measure to ensure that the warning is alway enabled despite previous errors/bad code.
There was a problem hiding this comment.
Only that you have to know where the error originates from to do that, which makes it a bit annoying. Actually, did we ever consider setting the warning to "always" for all warnings in the tests? That could decrease the probability of running in those kind of issues.... Next time I will run into it, which as far as I see is as soon as I want to waste more time on oindex related deprecations, hehe.
| In accordance with the Python C API, which gives preference to the half-open | ||
| interval over the closed one, ```np.random.random_integers``` is being | ||
| deprecated in favor of calling ```np.random.randint``` with the arguments | ||
| appropriately adjusted. Work has already been done to begin phasing out |
There was a problem hiding this comment.
Should probably mention that random_integers will not be removed anytime soon. Also, instead of saying that random_integers lacks the new features, point out that randint has new features not available in random_integers. Make it an endorsement of randint, not a put down of random_integers.
DEP: Deprecate random_integers
|
Thanks @gfyoung . |
|
It may also be worth removing references to What does this second sentence even mean? |
|
Not sure at the moment...you can submit a PR for I guess to clarify the meaning. |
Follow-up to the discussion #6910 in which to deprecate
np.random.random_integersin favor ofnp.random.randint.