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

Adding PDFs/CDFs to the main library #53

Open
ManifoldFR opened this issue Jul 17, 2023 · 5 comments
Open

Adding PDFs/CDFs to the main library #53

ManifoldFR opened this issue Jul 17, 2023 · 5 comments

Comments

@ManifoldFR
Copy link
Contributor

ManifoldFR commented Jul 17, 2023

Hi @bab2min ! Thanks for the great work you've done on this library. I'm using it for a purpose (importance sampling) where I need to compute the probability density or cumulative distribution functions of what I'm sampling.

I noticed some benchmarks implement some 1D pdfs and cdfs. Would you be interested in having those be part of the main library, as well as multivariate densities e.g. the multivariate normal density? For now what I've implemented is in my own client library, but I could make a PR if there's interest.

@bab2min
Copy link
Owner

bab2min commented Jul 17, 2023

Hi @ManifoldFR
Thank you for your suggestion. PR is always welcome.
It will be great if pdfs and cdfs were included in the library, especially separated header file.

@ManifoldFR
Copy link
Contributor Author

ManifoldFR commented Jul 17, 2023

Great! You'd prefer a separate header file where they are implemented as free functions? Not member functions of the XXGen (e.g. MvNormalGen) classes?
If you prefer the former I'll do that.

@bab2min
Copy link
Owner

bab2min commented Jul 17, 2023

@ManifoldFR
I thougt a free function would be good at first, but implementing as member functions looks better.
Is it correct that you are intending something like the following?

auto norm = NormalGen{0, 1};
norm.pdf(x) == exp(-x*x/2)/sqrt(2*pi)

@ManifoldFR
Copy link
Contributor Author

@ManifoldFR I thougt a free function would be good at first, but implementing as member functions looks better. Is it correct that you are intending something like the following?

auto norm = NormalGen{0, 1};
norm.pdf(x) == exp(-x*x/2)/sqrt(2*pi)

Yes, that's what I'm thinking of. We'd be replicating the API of scipy.stats: https://docs.scipy.org/doc/scipy/reference/generated/scipy.stats.multivariate_normal.html

It's also useful because in some classes such as MvNormalGen we already have a lower-triangular square root of the covariance matrix stored, and can use it to compute the determinant in the PDF

@bab2min
Copy link
Owner

bab2min commented Jul 17, 2023

@ManifoldFR Great! I cancel my words "separated header file". Please feel free to add member functions :)

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

No branches or pull requests

2 participants