-
Notifications
You must be signed in to change notification settings - Fork 6
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
added a uniform distribution over a collection of objects #19
Conversation
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 I'm misunderstanding something, but I don't think the pdf
corresponds to defined rand
.
src/distributions/uniform.jl
Outdated
function pdf(d::Uniform, s) | ||
d._set = something(d._set, Set(d.collection)) | ||
if s in d._set | ||
return 1/length(d.collection) |
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.
1.0/length(d.collection)
src/distributions/uniform.jl
Outdated
Uniform(c) = Uniform{typeof(c), eltype(c)}(c, nothing) | ||
Uniform(c::Set) = Uniform{typeof(c), eltype(c)}(c, c) | ||
|
||
rand(rng::AbstractRNG, d::Uniform) = rand(rng, d.collection) |
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.
Unclear about the rationale here. If you are allowing for non-sets and converting it into sets and giving them equal weights, then how can you sample uniformly from the collection which might have multiple instances of the same and therefore higher weight?
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.
Yeah, thanks, I need to redesign this.
Pull Request Test Coverage Report for Build 87
💛 - Coveralls |
@rejuvyesh do you think it's good now? |
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.
Just remove the commented stuff. Looks good to me otherwise.
src/distributions/uniform.jl
Outdated
# mean(d::Uniform) = mean(d.set) | ||
# mode(d::Uniform) = mode(d.set) | ||
# | ||
# function weighted_iterator(d::Uniform) |
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.
Delete these?
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.
oh yeah 🤦♂️
One question is naming. This clashes with
Distributions.Uniform
. Another option would beUniformCat
. I thinkUniform
is better, but could be persuaded otherwise.