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

Remove const-generic size parameter from Dirichlet distribution #15

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

MortenLohne
Copy link

  • Added a CHANGELOG.md entry

Summary

This PR is essentially a revert of rust-random/rand#1292, which changed the Dirichlet distribution to be const-generic over size. This broke my use case, which requires a Dirichlet distribution where the vector size is not known at compile time, see below.

Background

The original change was discussed in rust-random/rand#1292, #8 and rust-random/rand#496. I've also created #14 to discuss reverting that change.

The issue of requiring a statically known size doesn't seem to have been recognized at the time. It's only mentioned in a comment by @dhardy after the code was merged:

Caveat: the above approach does not support run-time variable length N. We likely don't need to care about this.

I don't think this is correct. A Dirichlet distribution over a dynamic number of elements was used in AlphaZero, one of the most widely read machine learning papers in the field. See the paper and this StackExchange post for details. This is essentially my use case, and given the impact of the paper, I'm sure many other people are doing this.

Additionally, it's not clear to me why the Dirichlet implementation needs to be const-generic at all, apart from a general desire to reduce the number of memory allocations. The performance hit is negligible for any use case I can imagine (feel free to prove me wrong here), and const-generic Dirichlet still requires an allocation, meaning it can't be used without the alloc feature.

Details

Even though the change was only released a month ago with 0.5, it was merged almost two years ago, and the code has changed substantially in the meantime. I removed the const generics manually, making as few other changes as possible, so it's possible that the code can now be simplified by someone with a better understand of statistics.

…wing it to be used with a dynamic number of parameters
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

Successfully merging this pull request may close these issues.

1 participant