Skip to content

ENH: Implement CDF of the bivariate normal distribution#112

Open
fbourgey wants to merge 6 commits intoscipy:mainfrom
fbourgey:bvnu
Open

ENH: Implement CDF of the bivariate normal distribution#112
fbourgey wants to merge 6 commits intoscipy:mainfrom
fbourgey:bvnu

Conversation

@fbourgey
Copy link
Copy Markdown
Member

Reference issue

Toward #98

What does this implement/fix?

Implement the CDF of the bivariate normal distribution from _bvnu

Additional information

I have kept the original comments (MATLAB code) from _bvnu

Copy link
Copy Markdown
Contributor

@dschmitz89 dschmitz89 left a comment

Choose a reason for hiding this comment

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

Thanks @fbourgey . The code looks correct. I have comments regarding readability/maintainability.

Could we rename the function for example? bvnu has the same charm as the ancient Fortran routines in amos. I think it was just taken over but why not use the chance to rename them to something more descriptive?

Comment thread include/xsf/stats.h Outdated
Comment thread include/xsf/stats.h Outdated
@fbourgey
Copy link
Copy Markdown
Member Author

@dschmitz89 thanks for the review! I have used a more explicit name and tried to factor out some of the functions. Let me know what you think.

@dschmitz89
Copy link
Copy Markdown
Contributor

dschmitz89 commented Apr 28, 2026

@dschmitz89 thanks for the review! I have used a more explicit name and tried to factor out some of the functions. Let me know what you think.

Thanks @fbourgey , imo the changes brought a big maintainability win. I will give this my approval. Are there any extra tests for the 2D case in SciPy itself?

@fbourgey
Copy link
Copy Markdown
Member Author

fbourgey commented Apr 28, 2026

@dschmitz89 thanks for the review! I have used a more explicit name and tried to factor out some of the functions. Let me know what you think.

Thanks @fbourgey , imo the changes brought a big maintainability win. I will give this my approval. Are there any extra tests for the 2D case in SciPy itself?

I found a few in test_multivariate.py. For example:

https://github.com/scipy/scipy/blob/main/scipy/stats/tests/test_multivariate.py#L887
https://github.com/scipy/scipy/blob/main/scipy/stats/tests/test_multivariate.py#L1016

@fbourgey
Copy link
Copy Markdown
Member Author

fbourgey commented May 2, 2026

@steppi @mdhaber what do you think of those changes?

@steppi
Copy link
Copy Markdown
Member

steppi commented May 2, 2026

@steppi @mdhaber what do you think of those changes?

It looks good from an algorithmic perspective but it's not there yet since it still relies on internal std::vector allocations that won't work with CUDA. I finally have scipy/scipy#24894 working now, so my recommendation is to pick one of your PRs and try to update it to work with SciPy by following that approach. My hope is that xsf will eventually only contain simple numerical kernels that work on both CPU and GPU with CPU specific code living in SciPy and GPU specific code living in CuPy.

Feel free to email me directly if you have any questions or need help with anything.

@fbourgey
Copy link
Copy Markdown
Member Author

fbourgey commented May 6, 2026

@steppi @mdhaber what do you think of those changes?

It looks good from an algorithmic perspective but it's not there yet since it still relies on internal std::vector allocations that won't work with CUDA. I finally have scipy/scipy#24894 working now, so my recommendation is to pick one of your PRs and try to update it to work with SciPy by following that approach. My hope is that xsf will eventually only contain simple numerical kernels that work on both CPU and GPU with CPU specific code living in SciPy and GPU specific code living in CuPy.

Feel free to email me directly if you have any questions or need help with anything.

@steppi @dschmitz89 I've tried to remove any std::vector and make the code CUDA compatible. Let me know your thoughts.

@steppi
Copy link
Copy Markdown
Member

steppi commented May 7, 2026

Oh very cool. I guess I didn't look closely enough and missed that the internal allocation was completely unnecessary. This won't even need the pattern from my recent poisson_binom SciPy PR. I can take a closer look after I work through my backlog a bit; or @dschmitz89 feel free to review and merge this one if it looks good to you.

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

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants