Enable AppVeyor, remove randi() #279
Conversation
|
@ararslan I've added a much more invasive commit. You may want to revise your approval... For future reference, the |
Nah, this is still fine by me. Any idea whether any packages that depend on StatsBase use that sampler directly? |
nalimilan
force-pushed
the
nl/appveyor
branch
2 times, most recently
from
1e92c22
to
1fe2397
Compare
Looking on GitHub, looks like there are a few uses, mostly in old, unmaintained packages. One exception is Distributions. So I've added deprecations which fix the tests for that package until we port it. The remaining failure on 32-bit 0.6 is unrelated, see JuliaLang/julia#22613. |
src/deprecates.jl
Outdated
|
|
||
| using Base.Random: RangeGenerator | ||
| @deprecate_binding RandIntSampler RangeGenerator | ||
| @deprecate RangeGenerator(K::Int) RangeGenerator(1:K) |
There was a problem hiding this comment.
I'm confused by this. Did you mean for the former to be RandIntSampler?
There was a problem hiding this comment.
That's a tricky situation. I first tried to do @deprecate RandIntSampler(K::Int) RangeGenerator(1:K), but that triggers a deprecation warning when loading StatsBase. So that's the only solution I could find. That's a kind of type piracy, but hopefully we won't have to keep this for too long.
There was a problem hiding this comment.
What if you move the binding deprecation to the end?
There was a problem hiding this comment.
Then I get a constant redefinition error (IIRC)...
There was a problem hiding this comment.
Hmmmmmmmmm, maybe
@deprecate (::Type{RandIntSampler})(K::Int) RangeGenerator(1:K)?
There was a problem hiding this comment.
If I don't use @deprecate_binding, the depwarn is not triggered, but I need to use const RandIntSampler = RangeGenerator, and then the methods are added to RangeGenerator. So that's equivalent to the current situation of the PR. If I only define RandIntSampler as a function, the places where it's used as a type will break (there's one case in Distributions, see JuliaStats/Distributions.jl#630). We can certainly do that, the question is whether it's better or worse than the type piracy.
There was a problem hiding this comment.
I guess another strategy would be to make a temporary type with a silly name like UseRangeGeneratorInstead, I think some of the linspace / float-range deprecations in base were done like that
There was a problem hiding this comment.
I'm not so concerned about the absence of a deprecation warning: it's unlikely that a package would use the type without any of the associated functions, which do print warnings. But however the type would be named, how would ensure at the same type that the old code continues to work (both code using the type and code using the functions)? AFAICT that requires type piracy.
There was a problem hiding this comment.
You'd make it an entirely new type, only for the purposes of keeping the deprecated code working. The warnings could say the right thing for the function calls, but the deprecate_binding warnings when it's used in type signatures would have the funny name.
There was a problem hiding this comment.
OK, actually the simplest solution seems to be moving the :RandIntSampler code to deprecates.jl, and printing deprecation warnings from the inner constructors. No type piracy involved. The use of the binding itself does not print a message, but you can't fix the warnings without removing the uses of the type anyway, and the only package which used this unexported type was Distributions. So should be good to go.
…andIntGenerator This fixes the inconsistency between 32-bit and 64-bit. rand() is somewhat slower than randi() when getting a single random number, but we should improve the implementation in Base if needed. Add deprecations since some packages used these internal methods.
|
What happened to |
|
See #281. |
|
Ah that's right, thanks for taking care of that! |
No description provided.