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

Migrate rand_distr to num-traits for no_std support #987

Merged
merged 16 commits into from
Aug 1, 2020
Merged

Conversation

newpavlov
Copy link
Member

@newpavlov newpavlov commented Jun 18, 2020

Closes: #922

If I am not mistaken, reproducibility tests on x86-64 pass without issues.

rand_distr/src/poisson.rs Outdated Show resolved Hide resolved
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.

Well spotted that since rand_distr is now an external crate, there's less pressure to minimise dependencies. But even so (and at risk of being accused of NIH), is there a good rationale for this change?

Aha, because num_traits provides trig functions not dependent on std / libm.

rand_distr/src/utils.rs Outdated Show resolved Hide resolved
rand_distr/src/lib.rs Outdated Show resolved Hide resolved
rand_distr/src/gamma.rs Outdated Show resolved Hide resolved
@dhardy
Copy link
Member

dhardy commented Jun 18, 2020

Looks like we'll need to bump the MSRV to 1.36; that's nearly a year old so probably fine. The latest Debian release only appears to support 1.34 however, so not so sure. But in any case the issue is only that the alloc crate was stabilised in 1.36 so we could simply not test that configuration in the MSRV CI config (and maybe add another config for 1.36+alloc).

Not sure what the other failure is. Lets retry later.

@newpavlov
Copy link
Member Author

newpavlov commented Jun 18, 2020

Looks like we'll need to bump the MSRV to 1.36

We could use the #[cfg(all(feature = "alloc", not(feature = "std")))] workaround, but personally I would prefer to bump MSRV, same goes for rand v0.8.

Probably we should synchronize rand_distr v0.3 and rand v0.8?

rand_distr/src/poisson.rs Outdated Show resolved Hide resolved
@newpavlov newpavlov changed the title Migrate rand_distr to num-traits Migrate rand_distr to num-traits for no_std support Jun 23, 2020
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.

This change looks good, especially the no_std support is something I wanted for a long time. I would like to see the potential undefined behavior from casting f64 to u64 resolved, with or without bringing back the u64 impl for Poisson.

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.

Regarding the MSRV, couldn't we simply change our testing to say the MSRV is 1.34 in general but 1.36 for the alloc feature? But if it gets complicated, then just bump to 1.36, agreed.

rand_distr/src/cauchy.rs Outdated Show resolved Hide resolved
@newpavlov
Copy link
Member Author

I've used the following workaround to get the old MSRV:

#[cfg(all(feature = "alloc", not(feature = "std")))]
extern crate alloc;
#[cfg(feature = "std")]
extern crate std;
// TODO: remove on MSRV bump to 1.36
#[cfg(feature = "std")]
extern crate std as alloc;

Also note that I've tweaked Dirichlet API a bit, I can revert the commit if you don't like the changes.

@vks
Copy link
Collaborator

vks commented Jun 25, 2020

Also note that I've tweaked Dirichlet API a bit, I can revert the commit if you don't like the changes.

What's the rationale for this change?

@newpavlov
Copy link
Member Author

Boxed slice is smaller than vector and all examples effectively use arrays for initialization (unfortunately I couldn't find real-world uses after a cursory github search), so I think it makes API simpler. Yes, it may result in an unnecessary allocation, but I doubt it will be common in practice.

Ideally it should use const generics and I think slice is closer to that than vector, so it could save some amount of churn during potential future migration.

@vks
Copy link
Collaborator

vks commented Jun 26, 2020

I just realized that this PR conflicts with another open PR, #958, which introduces support for usize for Poisson.

Boxed slice is smaller than vector and all examples effectively use arrays for initialization (unfortunately I couldn't find real-world uses after a cursory github search), so I think it makes API simpler. Yes, it may result in an unnecessary allocation, but I doubt it will be common in practice.

Fair enough!

Ideally it should use const generics and I think slice is closer to that than vector, so it could save some amount of churn during potential future migration.

I don't see how this would help with churn, switching to const generics will be a breaking change anyway.

@vks
Copy link
Collaborator

vks commented Jul 1, 2020

@newpavlov
Copy link
Member Author

I just realized that this PR conflicts with another open PR, #958, which introduces support for usize for Poisson.

I think my earlier argument still stands, so I think we should not merge it.

rand_distr/src/poisson.rs Outdated Show resolved Hide resolved
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.

This looks good to go to me (with the minor documentation change I suggested).

@dhardy Any concerns?

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.

For the most part this looks good.

Do you intend to squash or rebase this? It does several different things (such as moving value stability tests and the num-traits migration) which is quite a lot for one commit, but it is not desirable to keep the little fixes separate.

rand_distr/src/poisson.rs Outdated Show resolved Hide resolved
rand_distr/src/utils.rs Show resolved Hide resolved
@newpavlov
Copy link
Member Author

Do you intend to squash or rebase this?

Personally, I prefer squashing PRs.

rand_distr/Cargo.toml Outdated Show resolved Hide resolved
@vks
Copy link
Collaborator

vks commented Jul 31, 2020

I believe this can be merged after squashing and rebasing?

@dhardy
Copy link
Member

dhardy commented Aug 1, 2020

Yes. Does it need a rebase or just a squash? (I still think it's too much content for one commit, but nvm.)

@newpavlov
Copy link
Member Author

newpavlov commented Aug 1, 2020

I think simply pressing the "Squash and merge" button should be enough.

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.

rand_distr: no_std support
3 participants