-
Notifications
You must be signed in to change notification settings - Fork 0
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
Add diagrams for rand_distr
#1
Conversation
Unfortunately, it does not look like browsers can open SVGZ files (tested in Firefox and Chromium) without messing with |
BTW it may be worth to change the license from Apache 2.0 to CC-BY or CC-0. |
For the most part these plots look good. Can you increase the default size of the SVGs? At lest, opening these stand-alone, they are small. |
@newpavlov can you open a new issue regarding the licence? Also, we need a README, but not in this PR. |
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.
Nice that we can now review the plots in the web interface.
The only issue I spotted at a glance is two identical plots, but let me know if you want a more complete inspection.
(Already, this is a huge improvement from what we had.)
I have a few diagrams I know I still need to do / fix, which I'll work on today. I suggest the following: I will tick the checkboxes of any diagrams which are ready for review, and you (or I) can uncheck them again if there is potential for improvement 🤠 |
I also want to be sure, since these charts will be in the documentation, that they are consistent with the terminology used in the documentation with respect to naming (and potentially also examples). For instance, I noticed that the documentation for the Chi squared distribution calls the degrees of freedom parameter |
Also, I should ask: do we want plots of cumulative distribution functions as well, or would this be excessive? The workload would not be much more, and I'd be willing. |
Wikipedia does often show CDFs as well as PDFs, but since one is a simple translation of the other I don't think there's much value in both. @vks thoughts? |
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.
Reviewed:
- Beta
- Binomial
- Cauchy
- Chi squared
- Exponential, exp1
- Fisher
- Frechet
- Gamma: has six plots of uncoordinated colours; could e.g. saturation correspond to θ parameter?
- Geometric, standard geometric
- Gumbel
- Hypergeometric
- Inverse Gaussian
- Log normal
- Normal, standard normal
- Normal Inverse Gaussian: our implementation does not use scale or location parameters
- Pareto: three parametrisations is enough I think?
- Poisson
- Skew-normal
- Student's T
- Triangular
- Unit ball
- Unit circle: should not be solid
- Unit disc: should be solid
- Unit sphere: I suppose a wireframe is the best way to draw this...
- Weibull: we use parameters λ=scale, k=shape
- Zeta, Zipf
Thanks for the review. I've just started my Master's so I haven't had quite as much time to work on this but I'll implement your feedback and hopefully finally figure out how to get the Dirichlet code to work. |
@dhardy I've updated the diagrams which you gave feedback on, have a look:
Also, I added the Dirichlet distribution. I couldn't figure out how to do it on my own, so I eventually gave up and used some code I found on the internet, which I cited in |
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.
Some minor suggestions:
- It may be worth to enable grid for most plots. In my opinion, it makes plots easier to read.
- I would prefer for origin points to be at zero, right now they are slightly shifted.
- The Dirichlet plot lacks axis and color legend.
- Ideally, the plotted distributions should be generated by
rand
/rand_distr
. (It's not important and we can leave it for later)
Thanks for the review @newpavlov. I will add grid lines. |
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 that's all the major issues resolved, so I'll go ahead and approve. Thanks for your work on this.
from math import gamma | ||
|
||
|
||
# Code source: https://blog.bogatron.net/blog/2014/02/02/visualizing-dirichlet-distributions/ |
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.
Nice little article.
src/distributions/dirichlet.py
Outdated
|
||
def save_to(directory: str, _: str): | ||
extension = "png" # Hardcode png output format. SVG output for Dirichlet is ~22 MB, while png is ~115KB. | ||
corners = np.array([[0, 0], [1, 0], [0.5, 0.75 ** 0.5]]) |
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 notice that your plots use the same parameters as on Wikipedia but with a different coordinate layout. This is fine, but it would be nice to have some sort of labelling of axes on the plots.
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 agree, I haven't been able to figure out how to achieve that with matplotlib yet. I'll keep researching
|
Unlike |
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.
@MichaelOwenDyer is this ready? If so, please merge.
Sorry for the inactivity, I am still working on getting proper axes on the Dirichlet plot but have been so busy lately... will get it done soon |
Thanks for your patience @dhardy, I fixed the Dirichlet plot and I think it is ready to merge now :) |
I will merge it now, this PR has been open for long enough. Of course if we find any issues I will fix them in a follow-up |
Excellent! Thank you for these plots @MichaelOwenDyer. |
Summary
This PR adds some plots to the documentation of distributions in rand_distr to illustrate their behavior.
Motivation
Make the distributions in
rand
easier to understand via visual aids.Details
The
distributions
folder contains a separate Python file for each plot being created.main.py
is a script which saves all of the distribution plots into thecharts
folder.Used by rust-random/rand#1434.
Charts ready for review:
binomial::Binomial
cauchy::Cauchy
dirichlet::Dirichlet
exponential::Exp
exponential::Exp1
frechet::Frechet
gamma::Gamma
gamma::ChiSquared
gamma::FisherF
gamma::StudentT
gamma::Beta
geometric::Geometric
geometric::StandardGeometric
gumbel::Gumbel
hypergeometric::Hypergeometric
inverse_gaussian::InverseGaussian
normal::Normal
normal::StandardNormal
normal::LogNormal
normal_inverse_gaussian::NormalInverseGaussian
pareto::Pareto
pert::Pert
poisson::Poisson
skew_normal::SkewNormal
triangular::Triangular
unit_ball::UnitBall
unit_circle::UnitCircle
unit_disc::UnitDisc
unit_sphere::UnitSphere
weibull::Weibull
zipf::Zeta
zipf::Zipf