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

Place NdFloat and num-traits/std behind a new "std" feature #861

Merged
merged 1 commit into from
Dec 15, 2020

Conversation

xd009642
Copy link
Contributor

@xd009642 xd009642 commented Dec 13, 2020

This PR adds a new default feature "std" and when present this activates the "std" feature in num-traits and includes the NdFloat trait defined in ndarray. This is part of the tasks required in order to provide a no_std + alloc version of ndarray for issue #708

This is (an unavoidable) breaking change for any users that set default-features = false, other ndarray users will be unaffected.

@xd009642 xd009642 marked this pull request as draft December 13, 2020 19:57
@xd009642 xd009642 mentioned this pull request Dec 13, 2020
@xd009642
Copy link
Contributor Author

So this is designed as a PoC on a potential NdFloatCore with more minimal floating point requirements, as well as cfg'ing NdFloat behind a new std feature. This is part of the work/discussion to make ndarray no_std from here #708

@vadixidav
Copy link

vadixidav commented Dec 13, 2020

I don't see where the new std feature is added here (in Cargo.toml). Make sure you add that too.

@xd009642
Copy link
Contributor Author

Oops forgot to add to the commit. I ran a cargo fmt and it changed pretty much everything so missed the cargo.toml when adding just non-fmt changes 😬

@xd009642 xd009642 marked this pull request as ready for review December 14, 2020 07:14
@xd009642
Copy link
Contributor Author

Well tests pass so I guess I'll mark this as ready to review. As a summary of the motivation behind this PR.

It is some initial work for #708

This adds an std feature and makes the std feature of num-traits optional. It adds std to the default features so no breaking changes for crates that don't specify default-features = false, however for crates that do that and use NdFloat there will be a breaking change.

For the tests that use NdFloat I just changed them to use NdFloatCore for no_std tests. I'm aware this means the tests are using different traits for the std or no_std tests, but I think this is alright in the circumstance but if anyone disagrees it's an easy change for me to make 😄

@bluss
Copy link
Member

bluss commented Dec 14, 2020

This trait is only available with the std feature, or with the libm feature otherwise.

Num-traits Float says it can be used with no-std in this way. So we have exactly this problem: https://stackoverflow.com/questions/61769086/ (using std or libm but not both at the same time), with corresponding cargo feature request rust-lang/cargo#1839

@vadixidav
Copy link

vadixidav commented Dec 14, 2020

@bluss Now that you mention it, we should probably just require the libm feature. I think that the crate doesn't use libm if the std feature is present, but I haven't double checked yet. This would take care of this float problem.

@bluss
Copy link
Member

bluss commented Dec 14, 2020

We don't want to have the extra build time of libm if we don't use it.

src/linalg_traits.rs Outdated Show resolved Hide resolved
@xd009642
Copy link
Contributor Author

So @bluss I've removed NdFloatCore, and also made the oper tests f32 specifically so they'll still run without the std feature. I don't think it's worth using cfg to only run them for std given it's functionality that could exist on no_std as well

@bluss
Copy link
Member

bluss commented Dec 14, 2020

Thanks, makes sense to me to do this piecemeal, even though I made a suggestion make most of the PR disappear :) Could you update PR title and description before merge? I'd also rebase/squash it all together in this case I think (as you want). Lgtm.

@xd009642 xd009642 changed the title float core initial impl (breaking) Place NdFloat and num-traits/std behind a new "std" feature Dec 15, 2020
@xd009642
Copy link
Contributor Author

@bluss renamed, redescribed, and squashed 😄

@bluss
Copy link
Member

bluss commented Dec 15, 2020

Thanks!

@bluss
Copy link
Member

bluss commented Dec 15, 2020

You can merge it, since you can

@xd009642 xd009642 merged commit fa35a35 into rust-ndarray:master Dec 15, 2020
@xd009642
Copy link
Contributor Author

my first merge to ndarray 🎉

@xd009642 xd009642 deleted the float_core branch April 7, 2021 05:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants