chore: upgrade ndarray #42
Conversation
WalkthroughThe changes in this pull request involve updates to the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #42 +/- ##
==========================================
- Coverage 78.18% 75.30% -2.89%
==========================================
Files 4 4
Lines 486 486
==========================================
- Hits 380 366 -14
- Misses 106 120 +14 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
src/linalg/lapack.rs (6)
Line range hint
58-61: Consider Refactoring Duplicate Code inunsafe fnImplementationsThe implementations of
gelqf,gesdd,gesvd,heev, andunglqcontain similar patterns for workspace querying and allocation, error checking, and function calls. Refactoring this repeated code into a helper function could improve maintainability and reduce code duplication.Example of refactored code using a helper function:
unsafe fn workspace_query_and_call<F, G, T>( lapack_func: F, m: i32, n: i32, args: G, ) -> Result<T, i32> where F: Fn(/* appropriate parameters */), G: /* appropriate trait bounds */, T: /* appropriate return type */, { // Implement the common logic here }Then, update each
unsafe fnto use this helper function.Also applies to: 85-88, 112-115, 142-145, 169-172
Line range hint
198-204: Safety Concern: Use ofvec_uninitMay Lead to Undefined BehaviorThe
vec_uninitfunction creates a vector with uninitialized elements and sets its length without initializing its contents. This can lead to undefined behavior if any of the uninitialized elements are accessed before being properly initialized. Rust's safety guarantees are compromised when using uninitialized memory unsafely.Consider using
Vec::with_capacity(n)and pushing initialized values, or leveraging functions likevec![MaybeUninit::<T>::uninit(); n]along with proper initialization steps. Alternatively, for types that areCopy, you could usevec![T::zero(); n]if zero initialization is acceptable.Apply this diff to address the issue:
-unsafe fn vec_uninit<T: Sized>(n: usize) -> Vec<T> { - let mut v = Vec::with_capacity(n); - v.set_len(n); - v -} +use std::mem::MaybeUninit; + +unsafe fn vec_uninit<T: Sized>(n: usize) -> Vec<T> { + let v = Vec::<MaybeUninit<T>>::with_capacity(n); + // Ensure to properly initialize elements before use + // Implement initialization logic here + // Then, transmute Vec<MaybeUninit<T>> to Vec<T> if safe + v +}Ensure that all elements are properly initialized before any use to maintain memory safety.
Line range hint
31-38: Error Handling: Check Conversion fromSelftousizeThe usage of
to_usize().expect("valid integer")assumes that the conversion fromSelf(which could be a floating-point type likef32orf64) tousizewill always succeed. However, ifwork_size[0]contains a value that cannot be converted tousize, this will cause a panic at runtime.Consider adding proper error handling for the conversion to prevent potential panics.
Apply this diff to handle the conversion safely:
-let lwork = work_size[0].to_usize().expect("valid integer"); +let lwork = match work_size[0].to_usize() { + Some(val) => val, + None => return Err(-1), // Return an appropriate error code +};
1-5: Remove Unnecessary Clippy AllowancesThe file includes
#![allow(...)]for several Clippy lints (cast_sign_loss,cast_possible_truncation,cast_possible_wrap). If these casts are intentional and reviewed, consider adding explanations or targeting the allowances more narrowly to the specific code that requires them. Otherwise, remove them to ensure Clippy can catch potential issues.
Line range hint
7-10: Organize Imports Alphabetically and Group by CrateFor better readability and maintainability, consider organizing the import statements alphabetically and grouping them by crate origin.
Apply this diff to organize the imports:
use lair::Scalar; +use num_complex::{Complex32, Complex64}; use num_traits::{ToPrimitive, Zero}; use std::cmp; +use std::mem::MaybeUninit;
Line range hint
222-227: Typographical Error in CommentThere is a minor typo in the comment for
vec_uninit:"The caller must ensure that the elements of the vector are not used before they are initialized."
Consider rephrasing for clarity:
"The caller must ensure that the elements of the vector are properly initialized before use."
Apply this diff:
-/// they are initialized. +/// they are properly initialized before use.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
Cargo.toml(3 hunks)src/linalg/lapack.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Cargo.toml
🔇 Additional comments (1)
src/linalg/lapack.rs (1)
Line range hint 89-89: Potential Inconsistency in ld_a Parameter Passing
In the second call to the LAPACK functions within heev, gelqf, and unglq, the ld_a parameter is sometimes changed from ld_a to n. This could be intentional, but it may lead to unexpected behavior if ld_a is not equal to n.
Please verify that the ld_a parameter should be n in the second call. If this is not intentional, consider using ld_a consistently.
Run the following script to search for ld_a usage:
Also applies to: 116-116, 146-146
✅ Verification successful
The previous pattern might have been too specific. Let's broaden our search to find the LAPACK function calls and analyze the parameter patterns, particularly focusing on the mentioned functions heev, gelqf, and unglq.
Parameter Change in heev is Intentional and Correct
The search results confirm that the change from ld_a to n in the second call to heev is intentional and correct. This is because:
- The first call is a workspace query (with
-1work size) where the originalld_ais used to check the input matrix dimensions - The second call uses
nas the leading dimension because LAPACK'sheevoverwrites the input matrix with eigenvectors (whenjobz = 'V'), which requires a square matrix with leading dimension equal to the order of the matrix (n)
The same pattern is not present in gelqf and unglq where ld_a is consistently used, as these routines have different matrix layout requirements. The implementation is correct as is.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistency of `ld_a` parameter in LAPACK function calls.
# Search for lines where LAPACK functions are called and check `ld_a` usage.
rg -A 2 '\$[a-z]+\(.*,&mut a, (ld_a|n),' src/linalg/lapack.rs
Length of output: 63
Script:
#!/bin/bash
# Let's try a broader search pattern to find LAPACK function calls
rg -A 5 '\$[a-z]+\(' src/linalg/lapack.rs
# Also search specifically for heev, gelqf, and unglq functions
rg -A 10 '(heev|gelqf|unglq)' src/linalg/lapack.rs
Length of output: 7271
|
It appears that the |
Upgrade ndarray and transitive dependencies.