Skip to content

No_std support for ndarray #864

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

Merged
merged 22 commits into from
Jan 10, 2021
Merged

No_std support for ndarray #864

merged 22 commits into from
Jan 10, 2021

Conversation

SparrowLii
Copy link
Contributor

@SparrowLii SparrowLii commented Dec 17, 2020

This pr enables ndarray to be used in the no_std environment, and maintains most of the functions. Fixes #708.
The geomspace linspace logspace range var_axis and std_axis methods are only available when std is enabled at current. And 'blas''rayon''serde' features are not available without std too.

@SparrowLii
Copy link
Contributor Author

libcore cannot be used in blas-tests/oper.rs, which is a bit strange

@bluss
Copy link
Member

bluss commented Dec 17, 2020

Hm, do we need to start it this way? I'd suggest using the extern crate core as std solution, as I mentioned in the issue.

We need to figure out which tests to run in the no-std configuration. Crate test in itself depends on std, so we can keep using std in the test files.

@SparrowLii
Copy link
Contributor Author

In that case, it means that all dependencies on std in ndarray must be handled at once. Well, I will try that method.

@SparrowLii SparrowLii changed the title Replace the replaceable "std" statement in ndarray with "core" [WIP]Replace the replaceable "std" statement in ndarray with "core" Dec 18, 2020
@SparrowLii SparrowLii changed the title [WIP]Replace the replaceable "std" statement in ndarray with "core" [WIP] Replace the replaceable "std" statement in ndarray for no_std Dec 19, 2020
@SparrowLii
Copy link
Contributor Author

SparrowLii commented Dec 23, 2020

@bluss It seems that we need to resolve the dependencies of external cretas next. Do you think we should do it together here or in a new pr?

[Edit]They are mainly num-trait::Float and serde( and maybe others ). I can solve the former by using the limb feature in num-trait, while the latter is an optional package I haven’t changed for now.
My current goal is to first enable ndarray to be used in no_std environment, WDYT?

@SparrowLii SparrowLii changed the title [WIP] Replace the replaceable "std" statement in ndarray for no_std [WIP] no_std support for ndarray Dec 23, 2020
@SparrowLii
Copy link
Contributor Author

@bluss now it use FloatCore as Float in no_std environment. I think it will not affect the function under the std environment and can theoretically be used under no_std

@bluss
Copy link
Member

bluss commented Dec 24, 2020

@SparrowLii That kind of solution (FloatCore as Float) is not someting we can use in our public interface, it will create incompatibilities between our dependencies (it's a non-additive change to our API depending on features, which is not allowed).

crate features must be additive. When enabling std we can only enable more API, not change the definition of existing ones.

src/lib.rs Outdated
#[cfg(feature = "std")]
pub use num_traits::Float;
#[cfg(not(feature = "std"))]
pub use num_traits::float::FloatCore as Float;
Copy link
Member

Choose a reason for hiding this comment

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

This line must be deleted. Then we follow the consequence of that for not(feature="std").

Copy link
Member

Choose a reason for hiding this comment

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

If we need a creative solution, then we can "redirect" to support only f32, f64 on no-std (not general FloatCore!). Then we can feature-safely expand our methods bounded on A: Float to support the full A: Float range with std.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apologies for my lack of experience. I will change it.
I think it is cool to expand for FloatCore under no_std, but if we don’t use libm, we might have to implement exp() and ln() functions ourselves. I doubt about that.

@SparrowLii
Copy link
Contributor Author

If we need a creative solution, then we can "redirect" to support only f32, f64 on no-std (not general FloatCore!). Then we can feature-safely expand our methods bounded on A: Float to support the full A: Float range with std.

Apologies for my lack of experience. Have changed it.
I think it is cool to expand for FloatCore under no_std, but if we don’t use libm, we might have to implement exp() and ln() functions ourselves. I doubt about that.

@bluss
Copy link
Member

bluss commented Dec 24, 2020

Replacing the libm feature is not a goal no, don't worry about that.

@SparrowLii
Copy link
Contributor Author

SparrowLii commented Dec 24, 2020

Replacing the libm feature is not a goal no, don't worry about that.

Sorry for a little not understanding. So it means adding libm features or expand for FloatCore ourselves in ndarray?:)

@SparrowLii
Copy link
Contributor Author

Added libm features, so we can use --no-defualt-features --features "libm" to use all Float functions under no_std

@SparrowLii
Copy link
Contributor Author

@bluss any more advice? I think it can already be used under no_std. Maybe we should find a good way to verify?

@bluss
Copy link
Member

bluss commented Dec 29, 2020

I think this is a hard task if we try to do the impossible (making FloatCore/Float fit together). Let's follow a simple recipe which means that std is an additive feature. When std is disabled, some API is going to be missing because of that.

Anything else than that can be improved later.

@SparrowLii
Copy link
Contributor Author

I think this is a hard task if we try to do the impossible (making FloatCore/Float fit together). Let's follow a simple recipe which means that std is an additive feature. When std is disabled, some API is going to be missing because of that.

Anything else than that can be improved later.

Okay then, it should be better.

@SparrowLii
Copy link
Contributor Author

@bluss Now we avoid using the Float feature in the no_std environment. It can run under no_std. WDYT?

@bluss
Copy link
Member

bluss commented Jan 7, 2021

Great work, looks like it works. That's all we need for now. Features can be added back later.

  1. Do we have enough tests? There should be a test that ensures we build properly in no-std, not using std, and it should run in CI.
  2. Look at feature documentation. Features are documented in the module doc and in the README like this, and we need to add something for std. See link https://docs.rs/ndarray/0.14.0/ndarray/#crate-feature-flags
  3. We need to document on the features themselves. See in our docs you can find we say Requires crate feature "approx" on items that require that feature and so on. (There is an experimental Rust feature for nicer looking labels for this - but we don't need to use that feature yet if you don't want to - we can keep it simple and migrate everything to that later)
  4. Update the PR title, PR description and make them ready for merge. Update the description so that it properly says that it fixes the issue that it fixes, not just talks about it.
  5. Use interactive rebase to squash and clean up history (optional) - the PR commits are a bit messy - if you want to skip this, this change can be "squash merged", as one commit instead.

@SparrowLii SparrowLii changed the title [WIP] no_std support for ndarray No_std support for ndarray Jan 8, 2021
@SparrowLii
Copy link
Contributor Author

@bluss I updated the description of std feature and updated the pr description. But there are still two problems:
I did build test on linux-armv7(cargo build --target=thumbv7m-none-eabi --no-default-features)and it built successfully. But because I don’t know much about CI, I don’t know how to add it to the CI test script. I try to refer to the no_std of matrixmultiply, but there is no .travis.yml file in the ndarray.
I am happy to help optimize the document, but apart from adding a description of the std feature, I can’t find specific places to do it.
In addition, I tried to rebase, but encountered a lot of conflicts that I could not handle, so I thought it would be better to execute "squash merged".

README.rst Outdated
default `std` feature. Use this in `Cargo.toml`:

[dependencies]
ndarray = { version = "0.14.0", default-features = false }
Copy link
Member

Choose a reason for hiding this comment

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

Example with the version is nice but we should not use a specific version. Just more places to update when we bump the version. So we don't make an explicit example like this.

Copy link
Member

Choose a reason for hiding this comment

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

I think we also need to put the same update about feature flags into src/lib.rs - the module docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, has updated

@bluss bluss merged commit d27b1a9 into rust-ndarray:master Jan 10, 2021
@bluss
Copy link
Member

bluss commented Jan 10, 2021

Thanks! Finally done

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.

no_std for ndarray
2 participants