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

Use const generics in Dirichlet #1292

Merged
merged 2 commits into from
Mar 18, 2023
Merged

Conversation

Armavica
Copy link
Contributor

@Armavica Armavica commented Feb 24, 2023

This is a draft PR for issue #1006.

It is a working Dirichlet distribution that takes and returns const arrays, working with rand's current MSRV (1.56) [edit: but apparently not with Serde: serde-rs/serde/issues/1937].

It might still be useful to keep the original dynamic Dirichlet, so should I leave alone the original Dirichlet distribution and create a new one such as DirichletConst?

Copy link
Member

@dhardy dhardy left a comment

Choose a reason for hiding this comment

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

Seems like a good change.

Add a note to the CHANGELOG please.

rand_distr/src/dirichlet.rs Show resolved Hide resolved
rand_distr/src/dirichlet.rs Show resolved Hide resolved
@Armavica Armavica force-pushed the dirichlet-array branch 2 times, most recently from 448057c to 34bd7ef Compare March 2, 2023 01:04
@Armavica
Copy link
Contributor Author

Armavica commented Mar 2, 2023

serde won't serialize const arrays by default but I found a way to do it anyways with the crate serde_arrays: there might be other solutions which I would be happy to try if you want, let me know what you think!

@Armavica Armavica marked this pull request as ready for review March 2, 2023 04:14
@dhardy
Copy link
Member

dhardy commented Mar 2, 2023

The crate doesn't appear that well maintained. I'm guessing testing rand_distr with --no-default-features --features serde1 will fail now?

Perhaps an alternative is to serialise the array as a slice?

@Armavica
Copy link
Contributor Author

Armavica commented Mar 2, 2023

The crate doesn't appear that well maintained. I'm guessing testing rand_distr with --no-default-features --features serde1 will fail now?

No, in the current state of the PR, this testing configuration succeeds.

Perhaps an alternative is to serialise the array as a slice?

Sure, I can try that.

@Armavica
Copy link
Contributor Author

Armavica commented Mar 2, 2023

The crate doesn't appear that well maintained.

I found another crate which looks much more active: serde_with (PR updated).

I think that serializing as a slice would require to write our own serializing/deserializing code, right?

@dhardy
Copy link
Member

dhardy commented Mar 3, 2023

This looks okay, but I suggest serde1 should not imply serde_with. Instead, use serde_with as the feature-gate for serialization on Dirichlet.

@Armavica
Copy link
Contributor Author

Is this what you meant? I wasn't sure how the name the feature. I first tried to go with serde_with but apparently one cannot have a feature with the same name as the dependency. Or perhaps the feature name should mention Dirichlet or arrays?

@dhardy
Copy link
Member

dhardy commented Mar 16, 2023

but apparently one cannot have a feature with the same name as the dependency

You can. The old way to do this is just to have the optional feature in the Cargo.toml (you can still use feature = "serde_with" in code). The new way (since 1.60) is namespaced features, e.g. serde_with = ["dep:serde_with"] in the Cargo.toml. Our MSRV is still 1.56 so we have to use the former.

Copy link
Collaborator

@vks vks left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

@dhardy dhardy merged commit 0f5af66 into rust-random:master Mar 18, 2023
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.

3 participants