Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 24 additions & 24 deletions Cargo.toml
Copy link
Collaborator

Choose a reason for hiding this comment

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

To my knowledge, there is no Rust-mandated or even Rust-suggested TOML formatter, is there? So these changes could be undone as soon as I hit save on my Cargo.toml? If we want to enforce formatting for non-Rust files, we'll need to do it with something more standardized.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I use taplo: https://github.com/tamasfe/taplo

and just run taplo fmt in project and then integrate into CI.

To my knowledge, there is no Rust-mandated or even Rust-suggested TOML formatter, is there? So these changes could be undone as soon as I hit save on my Cargo.toml? If we want to enforce formatting for non-Rust files, we'll need to do it with something more standardized.

If you put it into the CI it would be standardized for all future PRs. I'm not sure its necessary for this project but I run it locally out of habit and see that it removed mostly extra whitespace. I don't know that changes would be undone if you changed the Cargo.toml though.

Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,7 @@ name = "ndarray"
version = "0.17.2"
edition = "2021"
rust-version = "1.64"
authors = [
"Ulrik Sverdrup \"bluss\"",
"Jim Turner"
]
authors = ["Ulrik Sverdrup \"bluss\"", "Jim Turner"]
license = "MIT OR Apache-2.0"
readme = "README-crates.io.md"

Expand All @@ -20,12 +17,12 @@ keywords = ["array", "data-structure", "multidimensional", "matrix", "blas"]
categories = ["data-structures", "science"]

include = [
"/src/**/*.rs",
"LICENSE-MIT",
"LICENSE-APACHE",
"RELEASES.md",
"README.rst",
"README-quick-start.md"
"/src/**/*.rs",
"LICENSE-MIT",
"LICENSE-APACHE",
"RELEASES.md",
"README.rst",
"README-quick-start.md",
]
resolver = "2"

Expand All @@ -46,9 +43,13 @@ rayon = { version = "1.10.0", optional = true }
cblas-sys = { workspace = true, optional = true }
libc = { version = "0.2.82", optional = true }

matrixmultiply = { version = "0.3.2", default-features = false, features=["cgemm"] }
matrixmultiply = { version = "0.3.2", default-features = false, features = [
"cgemm",
] }

serde = { version = "1.0", optional = true, default-features = false, features = ["alloc"] }
serde = { version = "1.0", optional = true, default-features = false, features = [
"alloc",
] }
rawpointer = { version = "0.2" }

[dev-dependencies]
Expand Down Expand Up @@ -77,20 +78,17 @@ portable-atomic-critical-section = ["portable-atomic/critical-section"]

[target.'cfg(not(target_has_atomic = "ptr"))'.dependencies]
portable-atomic = { version = "1.6.0" }
portable-atomic-util = { version = "0.2.0", features = [ "alloc" ] }
portable-atomic-util = { version = "0.2.0", features = ["alloc"] }

[workspace]
members = [
"ndarray-rand",
"crates/*",
]
members = ["ndarray-rand", "crates/*"]
default-members = [
".",
"ndarray-rand",
"crates/ndarray-gen",
"crates/numeric-tests",
"crates/serialization-tests",
# exclude blas-tests and blas-mock-tests that activate "blas" feature
".",
"ndarray-rand",
"crates/ndarray-gen",
"crates/numeric-tests",
"crates/serialization-tests",
# exclude blas-tests and blas-mock-tests that activate "blas" feature
]

[workspace.dependencies]
Expand All @@ -105,7 +103,9 @@ approx = { version = "0.5", default-features = false }
quickcheck = { version = "1.0", default-features = false }
rand = { version = "0.9.0", features = ["small_rng"] }
rand_distr = { version = "0.5.0" }
itertools = { version = "0.13.0", default-features = false, features = ["use_std"] }
itertools = { version = "0.13.0", default-features = false, features = [
"use_std",
] }
cblas-sys = { version = "0.1.4", default-features = false }

[profile.bench]
Expand Down
1 change: 0 additions & 1 deletion crates/blas-tests/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -39,4 +39,3 @@ accelerate = ["blas-src", "blas-src/accelerate"]
# Config for cargo-release
[package.metadata.release]
release = false

1 change: 0 additions & 1 deletion crates/ndarray-gen/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -11,4 +11,3 @@ num-traits = { workspace = true }
# Config for cargo-release
[package.metadata.release]
release = false

9 changes: 7 additions & 2 deletions crates/numeric-tests/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,13 @@ approx = { workspace = true }
rand = { workspace = true }
rand_distr = { workspace = true }

blas-src = { optional = true, version = "0.10", default-features = false, features = ["openblas"] }
openblas-src = { optional = true, version = ">=0.10.11", default-features = false, features = ["cblas", "system"] }
blas-src = { optional = true, version = "0.10", default-features = false, features = [
"openblas",
] }
openblas-src = { optional = true, version = ">=0.10.11", default-features = false, features = [
"cblas",
"system",
] }

[dev-dependencies]
num-traits = { workspace = true }
Expand Down
8 changes: 2 additions & 6 deletions examples/sort-axis.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,10 @@ pub struct Permutation
impl Permutation
{
/// Checks if the permutation is correct
pub fn from_indices(v: Vec<usize>) -> Result<Self, ()>
pub fn from_indices(v: Vec<usize>) -> Option<Self>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I understand that you didn't "change" anything, this is mostly reformatting, but this one here is different. This is a public function, so there might be several callers out there who will be surprised by this API change.

I'm not against changing it. I guess a Result<X, ()> is functionally equivalent to an Option<X>. I just want to highlight this change.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Personally I'm ok with this, but I'm glad you brought it up

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me know if you would like me to revert it.

https://effective-rust.com/transform.html

I think its better to return the Option and makes the signature clearer, and should require minimal refactoring downstream. While Result<T, ()> is basically the same functionally as Option<T> it doesn't covey the intent clearly. Usually Result is reserved for situations like where there is an expected value or an error variant and allows for propagating errors with ?.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agreed that it's the better practice, just good for us to keep in mind it will have to be a breaking change release.

{
let perm = Permutation { indices: v };
if perm.correct() {
Ok(perm)
} else {
Err(())
}
perm.correct().then_some(perm)
}

fn correct(&self) -> bool
Expand Down
11 changes: 5 additions & 6 deletions ndarray-rand/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,11 @@ description = "Constructors for randomized arrays. `rand` integration for `ndarr
keywords = ["multidimensional", "matrix", "rand", "ndarray"]

include = [
"/src/**/*.rs",
"LICENSE-MIT",
"LICENSE-APACHE",
"RELEASES.md",
"README.rst"
"/src/**/*.rs",
"LICENSE-MIT",
"LICENSE-APACHE",
"RELEASES.md",
"README.rst",
]

[dependencies]
Expand All @@ -34,4 +34,3 @@ quickcheck = { workspace = true }

[package.metadata.release]
tag-name = "ndarray-rand-{{version}}"

4 changes: 2 additions & 2 deletions rustfmt.toml
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ fn_call_width = 100
max_width = 120
brace_style = "AlwaysNextLine"
control_brace_style = "AlwaysSameLine"
fn_params_layout = "Compressed" # ?
fn_params_layout = "Compressed" # ?
format_macro_bodies = false
imports_granularity = "Preserve"
imports_indent = "Block"
Expand All @@ -16,7 +16,7 @@ match_arm_blocks = false
match_arm_leading_pipes = "Preserve"
merge_derives = false
overflow_delimited_expr = true
reorder_modules = false # impacts rustdoc order
reorder_modules = false # impacts rustdoc order
short_array_element_width_threshold = 32
skip_macro_invocations = ["*"]
unstable_features = true
Expand Down
16 changes: 8 additions & 8 deletions src/doc/crate_feature_flags.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,14 @@
//! ## `serde`
//! - Enables serialization support for serde 1.x
//!
//! ## `rayon`
//! - Enables parallel iterators, parallelized methods, the [`parallel`] module and [`par_azip!`].
//! - Implies std
#![cfg_attr(
not(feature = "rayon"),
doc = "//! ## `rayon`\n//! - Enables parallel iterators, parallelized methods, and the `par_azip!` macro.\n//! - Implies std\n"
)]
#![cfg_attr(
feature = "rayon",
doc = "//! ## `rayon`\n//! - Enables parallel iterators, parallelized methods, the [`crate::parallel`] module and [`crate::parallel::par_azip`].\n//! - Implies std\n"
)]
//!
//! ## `approx`
//! - Enables implementations of traits of the [`approx`] crate.
Expand All @@ -28,8 +33,3 @@
//!
//! ## `matrixmultiply-threading`
//! - Enable the ``threading`` feature in the matrixmultiply package
//!
//! [`parallel`]: crate::parallel

#[cfg(doc)]
use crate::parallel::par_azip;
13 changes: 10 additions & 3 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -84,11 +84,18 @@
//! ## Crate Feature Flags
//!
//! The following crate feature flags are available. They are configured in your
//! `Cargo.toml`. See [`doc::crate_feature_flags`] for more information.
//! `Cargo.toml`. See [`crate::doc::crate_feature_flags`] for more information.
//!
//! - `std`: Rust standard library-using functionality (enabled by default)
//! - `serde`: serialization support for serde 1.x
//! - `rayon`: Parallel iterators, parallelized methods, the [`parallel`] module and [`par_azip!`].
#![cfg_attr(
not(feature = "rayon"),
doc = "//! - `rayon`: Parallel iterators, parallelized methods, and the `par_azip!` macro."
)]
#![cfg_attr(
feature = "rayon",
doc = "//! - `rayon`: Parallel iterators, parallelized methods, the [`parallel`] module and [`par_azip!`]."
)]
//! - `approx` Implementations of traits from the [`approx`] crate.
//! - `blas`: transparent BLAS support for matrix multiplication, needs configuration.
//! - `matrixmultiply-threading`: Use threading from `matrixmultiply`.
Expand Down Expand Up @@ -129,7 +136,7 @@ extern crate std;
#[cfg(feature = "blas")]
extern crate cblas_sys;

#[cfg(docsrs)]
#[cfg(any(doc, docsrs))]
pub mod doc;

use alloc::fmt::Debug;
Expand Down
114 changes: 57 additions & 57 deletions src/linalg/impl_linalg.rs
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why did a large block of code move in this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Modules shouldn't be implemented after tests: https://rust-lang.github.io/rust-clippy/master/index.html#items_after_test_module

error: items after a test module
--> src/linalg/impl_linalg.rs:973:1
|
973 | mod blas_tests
| ^^^^^^^^^^^^^^
...
1104 | / impl Dot<ArrayRef<A, IxDyn>> for ArrayRef<A, IxDyn>
1105 | | where A: LinalgScalar
| |_____________________^
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#items_after_test_module
= note: -D clippy::items-after-test-module implied by -D warnings
= help: to override -D warnings add #[allow(clippy::items_after_test_module)]
= help: move the items to before the test module was defined

error: could not compile ndarray (lib test) due to 1 previous error

Original file line number Diff line number Diff line change
Expand Up @@ -968,6 +968,63 @@ where
is_blas_2d(a._dim(), a._strides(), BlasOrder::F)
}

/// Dot product for dynamic-dimensional arrays (`ArrayD`).
///
/// For one-dimensional arrays, computes the vector dot product, which is the sum
/// of the elementwise products (no conjugation of complex operands).
/// Both arrays must have the same length.
///
/// For two-dimensional arrays, performs matrix multiplication. The array shapes
/// must be compatible in the following ways:
/// - If `self` is *M* × *N*, then `rhs` must be *N* × *K* for matrix-matrix multiplication
/// - If `self` is *M* × *N* and `rhs` is *N*, returns a vector of length *M*
/// - If `self` is *M* and `rhs` is *M* × *N*, returns a vector of length *N*
/// - If both arrays are one-dimensional of length *N*, returns a scalar
///
/// **Panics** if:
/// - The arrays have dimensions other than 1 or 2
/// - The array shapes are incompatible for the operation
/// - For vector dot product: the vectors have different lengths
impl<A> Dot<ArrayRef<A, IxDyn>> for ArrayRef<A, IxDyn>
where A: LinalgScalar
{
type Output = Array<A, IxDyn>;

fn dot(&self, rhs: &ArrayRef<A, IxDyn>) -> Self::Output
{
match (self.ndim(), rhs.ndim()) {
(1, 1) => {
let a = self.view().into_dimensionality::<Ix1>().unwrap();
let b = rhs.view().into_dimensionality::<Ix1>().unwrap();
let result = a.dot(&b);
ArrayD::from_elem(vec![], result)
}
(2, 2) => {
// Matrix-matrix multiplication
let a = self.view().into_dimensionality::<Ix2>().unwrap();
let b = rhs.view().into_dimensionality::<Ix2>().unwrap();
let result = a.dot(&b);
result.into_dimensionality::<IxDyn>().unwrap()
}
(2, 1) => {
// Matrix-vector multiplication
let a = self.view().into_dimensionality::<Ix2>().unwrap();
let b = rhs.view().into_dimensionality::<Ix1>().unwrap();
let result = a.dot(&b);
result.into_dimensionality::<IxDyn>().unwrap()
}
(1, 2) => {
// Vector-matrix multiplication
let a = self.view().into_dimensionality::<Ix1>().unwrap();
let b = rhs.view().into_dimensionality::<Ix2>().unwrap();
let result = a.dot(&b);
result.into_dimensionality::<IxDyn>().unwrap()
}
_ => panic!("Dot product for ArrayD is only supported for 1D and 2D arrays"),
}
}
}

#[cfg(test)]
#[cfg(feature = "blas")]
mod blas_tests
Expand Down Expand Up @@ -1083,60 +1140,3 @@ mod blas_tests
}
}
}

/// Dot product for dynamic-dimensional arrays (`ArrayD`).
///
/// For one-dimensional arrays, computes the vector dot product, which is the sum
/// of the elementwise products (no conjugation of complex operands).
/// Both arrays must have the same length.
///
/// For two-dimensional arrays, performs matrix multiplication. The array shapes
/// must be compatible in the following ways:
/// - If `self` is *M* × *N*, then `rhs` must be *N* × *K* for matrix-matrix multiplication
/// - If `self` is *M* × *N* and `rhs` is *N*, returns a vector of length *M*
/// - If `self` is *M* and `rhs` is *M* × *N*, returns a vector of length *N*
/// - If both arrays are one-dimensional of length *N*, returns a scalar
///
/// **Panics** if:
/// - The arrays have dimensions other than 1 or 2
/// - The array shapes are incompatible for the operation
/// - For vector dot product: the vectors have different lengths
impl<A> Dot<ArrayRef<A, IxDyn>> for ArrayRef<A, IxDyn>
where A: LinalgScalar
{
type Output = Array<A, IxDyn>;

fn dot(&self, rhs: &ArrayRef<A, IxDyn>) -> Self::Output
{
match (self.ndim(), rhs.ndim()) {
(1, 1) => {
let a = self.view().into_dimensionality::<Ix1>().unwrap();
let b = rhs.view().into_dimensionality::<Ix1>().unwrap();
let result = a.dot(&b);
ArrayD::from_elem(vec![], result)
}
(2, 2) => {
// Matrix-matrix multiplication
let a = self.view().into_dimensionality::<Ix2>().unwrap();
let b = rhs.view().into_dimensionality::<Ix2>().unwrap();
let result = a.dot(&b);
result.into_dimensionality::<IxDyn>().unwrap()
}
(2, 1) => {
// Matrix-vector multiplication
let a = self.view().into_dimensionality::<Ix2>().unwrap();
let b = rhs.view().into_dimensionality::<Ix1>().unwrap();
let result = a.dot(&b);
result.into_dimensionality::<IxDyn>().unwrap()
}
(1, 2) => {
// Vector-matrix multiplication
let a = self.view().into_dimensionality::<Ix1>().unwrap();
let b = rhs.view().into_dimensionality::<Ix2>().unwrap();
let result = a.dot(&b);
result.into_dimensionality::<IxDyn>().unwrap()
}
_ => panic!("Dot product for ArrayD is only supported for 1D and 2D arrays"),
}
}
}
3 changes: 3 additions & 0 deletions src/partial.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ impl<T> Partial<T>
}

#[cfg(feature = "rayon")]
#[allow(dead_code)]
pub(crate) fn stub() -> Self
{
Self {
Expand All @@ -46,6 +47,7 @@ impl<T> Partial<T>
}

#[cfg(feature = "rayon")]
#[allow(dead_code)]
pub(crate) fn is_stub(&self) -> bool
{
self.ptr.is_null()
Expand All @@ -60,6 +62,7 @@ impl<T> Partial<T>
}

#[cfg(feature = "rayon")]
#[allow(dead_code)]
/// Merge if they are in order (left to right) and contiguous.
/// Skips merge if T does not need drop.
pub(crate) fn try_merge(mut left: Self, right: Self) -> Self
Copy link
Collaborator

Choose a reason for hiding this comment

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

Under what check is this dead code? I can clearly see that it is called inside the zip_impl macro

Copy link
Collaborator

Choose a reason for hiding this comment

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

Same question for several other parallel-related dead code annotations

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Under what check is this dead code? I can clearly see that it is called inside the zip_impl macro

It is feature gated:

    #[cfg(feature = "rayon")]
    #[allow(dead_code)]

https://doc.rust-lang.org/std/macro.cfg.html

In Cargo.toml:

rayon = { version = "1.10.0", optional = true }

Rayon is an optional feature and therefore used when added at compile time, I was following the convention here, though it may not be necessary from your feedbacK?

#[allow(dead_code)] // used only when Rayon support is enabled

Happy to remove them. Please let me know.

Expand Down
1 change: 1 addition & 0 deletions src/zip/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -424,6 +424,7 @@ where D: Dimension
}

#[cfg(feature = "rayon")]
#[allow(dead_code)]
pub(crate) fn uninitialized_for_current_layout<T>(&self) -> Array<MaybeUninit<T>, D>
{
let is_f = self.prefer_f();
Expand Down
Loading