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

DOC: Kernel density notebook update #4207

Closed
wants to merge 6 commits into from
Closed

DOC: Kernel density notebook update #4207

wants to merge 6 commits into from

Conversation

tommyod
Copy link
Contributor

@tommyod tommyod commented Jan 14, 2018

First PR to statsmodels. The Jupyter Notebook on Kernel Density Estimation seemed very incomplete to me, so I updated it with the following:

  • More text and comment explaining the code.
  • Example comparing a true distribution to the KDE of the sampled distribution.
  • The effect of varying the bw argument (the bandwidth of the kernel).
  • The various kernel functions available (one plot for each of them).
  • Cleaned up the plots a bit: smaller, with grids, default maplotlib colors instead of black/red.

I also fixed a minor typo.

Comments very welcome!

@coveralls
Copy link

Coverage Status

Coverage remained the same at 82.13% when pulling cd9b8cf on tommyod:kernel_density into 4151898 on statsmodels:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 82.13% when pulling cd9b8cf on tommyod:kernel_density into 4151898 on statsmodels:master.

@josef-pkt
Copy link
Member

@tommyod Thanks for the improvement

I only had time for a very quick skimming of it.
Looks good overall
content of notebook will need to be removed before merging

the only comment directly to the content is that icdf doesn't fit in the plot with cdf and sf.
The x-axis for icdf is misleading because it should be the interval [0, 1]

@codecov-io
Copy link

codecov-io commented Jan 14, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@91ed779). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #4207   +/-   ##
=========================================
  Coverage          ?   79.55%           
=========================================
  Files             ?      562           
  Lines             ?    83816           
  Branches          ?     9551           
=========================================
  Hits              ?    66683           
  Misses            ?    14947           
  Partials          ?     2186

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 91ed779...b357286. Read the comment docs.

@coveralls
Copy link

coveralls commented Jan 20, 2018

Coverage Status

Changes Unknown when pulling d475bf4 on tommyod:kernel_density into ** on statsmodels:master**.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling b357286 on tommyod:kernel_density into ** on statsmodels:master**.

1 similar comment
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling b357286 on tommyod:kernel_density into ** on statsmodels:master**.

@tommyod
Copy link
Contributor Author

tommyod commented Jan 20, 2018

@josef-pkt Thanks for the review. I read through again and wrote a little bit more. I removed the output of the Jupyter Notebook, and addressed the comment about the domain of the inverse CDF as per your request :)

@josef-pkt josef-pkt added this to the 0.9 milestone Apr 19, 2018
@josef-pkt josef-pkt mentioned this pull request Apr 22, 2018
Copy link
Member

@josef-pkt josef-pkt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think using alpha=0.5 for the histograms except the first would look better, better visibility of plot lines on top of histogram

"dist1_loc, dist1_scale, weight1 = -1 , .5, .25\n",
"dist2_loc, dist2_scale, weight2 = 1 , .5, .75\n",
"\n",
"# Sample from a micture of distributions\n",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo mixture

@josef-pkt
Copy link
Member

A squashed and rebased version of this is now in #4551

@josef-pkt
Copy link
Member

merged in #4551

@tommyod Thanks for the improvement

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants