Skip to content
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

Replace sampletype with Random.gentype #214

Closed
zsunberg opened this issue Sep 17, 2018 · 2 comments
Closed

Replace sampletype with Random.gentype #214

zsunberg opened this issue Sep 17, 2018 · 2 comments
Labels
decision-made This decision has been settled
Milestone

Comments

@zsunberg
Copy link
Member

just noticed that this is the standard

@lassepe
Copy link
Member

lassepe commented Sep 9, 2019

To my understanding, Random.gentype has not been documented on purpose, because it is unclear at this point whether it should be part of the official API. In fact, I can't find any documentation of it in the v1.2 docs. It looks like this is still in some transient phase and people are leaning toward using eltype instead.

Distributions.jl suggests that people should use eltype as well; quote:

By default, Discrete sampleables have support of type Int while Continuous sampleables have support of type Float64. If this assumption does not hold for your new distribution or sampler, or its ValueSupport is neither Discrete nor Continuous, you should implement the eltype method in addition to the other methods listed below.

Maybe we should suggest to implement the Distributions.jl Sampleable interface?

@zsunberg
Copy link
Member Author

Actually, now I am leaning towards just getting rid of it.

It doesn't seem like we actually use it anywhere: https://github.com/search?p=2&q=org%3AJuliaPOMDP+sampletype&type=Code, and it is probably actually safer to let the compiler infer it than to make people write it.

@zsunberg zsunberg added decision decision-made This decision has been settled and removed decision labels Sep 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
decision-made This decision has been settled
Projects
None yet
Development

No branches or pull requests

2 participants