Thanks for the quick and kind review! @nilgoyette I just added docstring and removed length check which is redundant.

I don't know if the other maintainers reached a decision concerning the "formatting problem", so I'm unsure if your last commit is acceptable or not. Personally, I would run a cargo fmt on the whole project and never talk about this problem again, but it's a little more complex than that :) You can wait for their input, or you can revert your last commit (if you want the PR to be merged faster). Up to you.

I don't know if the other maintainers reached a decision concerning the "formatting problem", so I'm unsure if your last commit is acceptable or not. Personally, I would run a cargo fmt on the whole project and never talk about this problem again, but it's a little more complex than that :) You can wait for their input, or you can revert your last commit (if you want the PR to be merged faster). Up to you.

oh, I got it! thanks to kind explanation! I just undo the last commit(cargo fmt) 👍

Unfortunately our current formatting settings require cargo +nightly fmt, since many of the formatting features we use are unstable. I am starting to think that the barrier to new PRs presented by a nightly-only cargo fmt is not worth the pain of the single formatting commit that would be necessary to move us to a stable-only cargo fmt.

Btw this PR looks great! Let me fix the CI on our master branch and retrigger this CI run. We can discuss the whole cargo fmt issue, but perhaps you could just run cargo +nightly fmt and we can get this merged in?

@akern40 Good morning(or evening)! cargo +nightly fmt commit included! iirc, there's guide to use nightly cargo for ndarray, I think I missed it.
I know many cool features are in cargo nightly, so it would be nice to stick to cargo +nightly fmt, imo.

@akern40 Sorry to bother you, but some of the integrated tests still went to fail. I read your PR which is merged(#1485 ), and some of the packages need higher rust version, I guess.

for example,

error: package `zerofrom v0.1.6` cannot be built because it requires rustc 1.81 or newer, while the currently active rustc version is 1.71.1

hope this is helpful to you!

other changes

• I added no_run tag to not to run example in docstring for CI.
• add vec! macro in use block
• merge master

Huh, that's very odd! Can you make sure that you have rebased / pulled the latest commits from main and run cargo update --locked to make sure you are on the same dependencies? Also, please remove the no_run from the example, we do want examples run in the CI.

(Also you're not bothering me! This is what being a maintainer is all about - making sure others can successfully commit to the library)

Huh, that's very odd! Can you make sure that you have rebased / pulled the latest commits from main and run cargo update --locked to make sure you are on the same dependencies? Also, please remove the no_run from the example, we do want examples run in the CI.

@akern40 thanks! I forgot pull master before I push the changes 😅

@akern40 ping you again because some tests went fail due to examples in docstring 😥 fixed it, now it's fine with cargo test --doc

Ok I think I understand what's happening here, after a bit of testing / reading: the tests you have written work fine without blas, as expected. However, the way blas works with ndarray right now is that two things must occur: ndarray must be built with the blas feature, and the binary (in our case, the test binary) must be built by selecting a bias-src and using extern crate blas. So your tests - which always run - have prerequisite #1 completed, but not prerequisite #2. I believe the solution is to move these tests into crates/blas-tests/tests/oper.rs (or, maybe better, a new file: crates/blas-tests/tests/dyn.rs). This crate is set up to do both prerequisite steps.

Ok I think I understand what's happening here, after a bit of testing / reading: the tests you have written work fine without blas, as expected. However, the way blas works with ndarray right now is that two things must occur: ndarray must be built with the blas feature, and the binary (in our case, the test binary) must be built by selecting a bias-src and using extern crate blas. So your tests - which always run - have prerequisite #1 completed, but not prerequisite #2. I believe the solution is to move these tests into crates/blas-tests/tests/oper.rs (or, maybe better, a new file: crates/blas-tests/tests/dyn.rs). This crate is set up to do both prerequisite steps.

@akern40 wow, thanks! I didn't know that. I just made new commit to follow your suggestion.
btw, before push crates/blas-tests/tests/dyn.rs, what if I use this #[cfg(feature = "blas")] on my test? other tests in impl_linalg.rs are using it. (if this is intended to run blas backend. )

for example,

#[cfg(test)]
#[cfg(feature = "blas")]
fn blas_column_major_2d<A, S>(a: &ArrayBase<S, Ix2>) -> bool
where
    S: Data,
    A: 'static,
    S::Elem: 'static,
{
    if !same_type::<A, S::Elem>() {
        return false;
    }
    is_blas_2d(&a.dim, &a.strides, BlasOrder::F)
}

It's a little misleading, but if you take a close look at the tests that are currently in impl_linalg, you'll notice that they're testing the surrounding machinery of the BLAS setup, but not the BLAS code itself. So when blas is enabled, this code tests the non-BLAS-calling code we write to support it, but it doesn't depend on the extern blas_src that is required for actually executing BLAS operations.

So, in short, your code would still have to go into dyn.rs, sorry!

Btw, thank you for this - it's a good opportunity for me to really wrap my head around the BLAS situation here. I'm starting in on some ideas to make this all easier, hopefully. We'll see if any of the ideas work 😄

@akern40 moved tests crates/blas-tests/tests/dyn.rs.
I checked it with my own machine with blas integration, hope it works fine with machine on CI pipeline!

Thanks to kind and clear guide to contribute!

@NewBornRustacean thank you for your pull request, your patience, and your collaboration, and congratulations on your first PR to ndarray! I hope it will be the first of many.

@akern40 @nilgoyette thank you all! want to contribute more obviously😁 will be back with next pr 🫡

Please release 0.17 with this patch.