Repository navigation
feat(linalg): implement Display for VecView and VecMutView - #495
Conversation
|
Thanks for the focused change. The concrete implementations are straightforward, and using slice Debug formatting matches the existing ArrayView2::display row format without collecting another Vec. However, I would address the following before merging. 1. The public slicing API still cannot use DisplayArray1::slice returns Box<dyn ArrayView1>, and slice_mut returns Box<dyn MutArrayView1>. Neither trait exposes Display, so the new implementations on VecView/VecMutView are erased at those API boundaries. For example, this still fails to compile: use smartcore::linalg::basic::arrays::Array1;
let x = vec![1, 2, 3, 4];
let view = x.slice(1..3);
assert_eq!(format!("{view}"), "[2, 3]");The same applies to slice_mut. The new test does not catch this because it constructs the concrete types directly through private ptr fields; downstream users cannot construct them that way. Please cover formatting through both public slicing methods and provide formatting for their returned trait objects. One additive option is Display implementations for dyn ArrayView1 + '_ and dyn MutArrayView1 + '_, using iterator(0) and streaming into the Formatter without collecting elements. Avoid simply adding Display as a supertrait: existing implementations for Vec and &[T] cannot satisfy that requirement because of the orphan rule. 3. Regression coverage and issue scopeOnce the public-API case above works, please also cover empty and singleton views and at least one floating-point case, for both immutable and mutable views. Integer-only tests do not demonstrate the intended Debug-style element representation for values such as 1.0. Please also clarify the scope of Fixes #197: this change adds formatting for concrete 1D view types, but does not yet make the returned 1D trait objects printable with {}. If broader array formatting remains out of scope, use a non-closing reference until the issue's acceptance criteria are confirmed. Review scope: static inspection of head 3eed340, the full changed file, relevant array traits, and issue #197; I did not run the Rust build/tests. At the time checked, 9 CI jobs had passed and 4 were still running, with no reported failures. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #495 +/- ##
===========================================
+ Coverage 43.97% 85.58% +41.60%
===========================================
Files 85 96 +11
Lines 7281 9373 +2092
===========================================
+ Hits 3202 8022 +4820
+ Misses 4079 1351 -2728 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Follow-up to my earlier review. Zero-copy note on the Display impls: implementing Display does not materialise the views, as long as the implementation streams elements into the Formatter and never collects them. The PR's current write!(f, "{:?}", self.ptr) satisfies that. Please keep any trait-object implementation streaming as well. Note that ArrayView2::display collects each row into a Vec before printing, so I would not copy that pattern. Suggested form: f.debug_list().entries(self.iterator(0)).finish(). It avoids copying elements, and it preserves the Formatter flags (for example {:.2} precision), which the nested write!("{:?}") discards. iterator(0) returns a Box, so the only allocation is that small boxed iterator, not the data. Element format: elements are printed with Debug, although T also has a Display bound. These differ for floats (1.0f64 prints as 1.0 under Debug and 1 under Display). Please state which is intended and cover it with a test. |
|
Re-review of head 66b462c. The earlier code and test points are addressed:
CI is now fully green: all 15 checks passed, including wasm32, macOS, Windows x86_64, coverage and codecov. I reviewed the diff statically and did not build or run the tests locally. Remaining small items, none blocking:
On Fixes #197: Vec and &[T] remain unprintable via {} because of the orphan rule, as the description notes, so I would leave it to the maintainers whether that fully closes the issue. |
Fixes #197
Checklist
Current behaviour
DenseMatrix,DenseMatrixViewandDenseMatrixMutViewalready implementstd::fmt::Display, soprintln!("{}", m)works for the 2D arrays. The 1D array views inlinalg::basic::vector(VecViewandVecMutView, returned byArray1::slice/slice_muton aVec) do not implementDisplay. Onlyprintln!("{:?}", v)works for them, and that prints the inner struct (VecView { ptr: [..] }).New expected behaviour
VecViewandVecMutViewimplementDisplay. They print their elements as a list, for example[2, 3]. This is the same row format thatArrayView2::displayuses forDenseMatrix. With this change, all array types that smartcore itself defines support{}.Vec<T>and&[T]are foreign types, so the orphan rule prevents aDisplayimpl for them.Change logs
Added
impl Display for VecViewandimpl Display for VecMutViewinsrc/linalg/basic/vector.rs.test_displayunit test in the same file. It does not compile without the new impls.Testing
cargo test --lib linalg::basic::vector(fails to compile before the change with E0277, passes after)cargo test --all-featurescargo fmt --all -- --checkcargo clippy --all-features -- -Drust-2018-idioms -Dwarnings