Skip to content

feat(linalg): implement Display for VecView and VecMutView - #495

Merged
Mec-iS merged 2 commits into
smartcorelib:mainfrom
ShryukGrandhi:fix/issue-197-define-default-formatter-for-linalg-basi
Oct 20, 2026
Merged

Mec-iS merged 2 commits into
smartcorelib:mainfrom
ShryukGrandhi:fix/issue-197-define-default-formatter-for-linalg-basi

Conversation

@ShryukGrandhi

Copy link
Copy Markdown
Contributor

Fixes #197

Checklist

  • My branch is up-to-date with main branch.
  • Everything works and tested on latest stable Rust.
  • Coverage and Linting have been applied

Current behaviour

DenseMatrix, DenseMatrixView and DenseMatrixMutView already implement std::fmt::Display, so println!("{}", m) works for the 2D arrays. The 1D array views in linalg::basic::vector (VecView and VecMutView, returned by Array1::slice / slice_mut on a Vec) do not implement Display. Only println!("{:?}", v) works for them, and that prints the inner struct (VecView { ptr: [..] }).

New expected behaviour

VecView and VecMutView implement Display. They print their elements as a list, for example [2, 3]. This is the same row format that ArrayView2::display uses for DenseMatrix. With this change, all array types that smartcore itself defines support {}. Vec<T> and &[T] are foreign types, so the orphan rule prevents a Display impl for them.

Change logs

Added

  • impl Display for VecView and impl Display for VecMutView in src/linalg/basic/vector.rs.
  • test_display unit 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-features
  • cargo fmt --all -- --check
  • cargo clippy --all-features -- -Drust-2018-idioms -Dwarnings

@ShryukGrandhi
ShryukGrandhi requested a review from Mec-iS as a code owner October 20, 2026 07:13
@Mec-iS

Mec-iS commented Oct 20, 2026 •

Copy link
Copy Markdown
Collaborator

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 Display

Array1::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 scope

Once 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

codecov Bot commented Oct 20, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.58%. Comparing base (9eaae9e) to head (66b462c).
⚠️ Report is 202 commits behind head on main.

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Mec-iS

Mec-iS commented Oct 20, 2026 •

Copy link
Copy Markdown
Collaborator

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.

@Mec-iS

Mec-iS commented Oct 20, 2026

Copy link
Copy Markdown
Collaborator

Re-review of head 66b462c.

The earlier code and test points are addressed:

  • dyn ArrayView1 and dyn MutArrayView1 now implement Display, so the Box returned by slice() and slice_mut() can be formatted with {}.
  • The implementations stream through f.debug_list().entries(self.iterator(0)).finish(), so elements are not copied and Formatter flags such as {:.2} propagate.
  • The new tests go through the public slice()/slice_mut() API and cover empty, singleton and float cases, including Debug formatting (1.0 rather than 1) and {:.2} precision.

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:

  1. The PR description is still stale. It says the output is "the same row format that ArrayView2::display uses", but that method prints one row per line, whereas this prints a single bracketed list. The Added list omits the two dyn impls, which are the public API additions that make slice()/slice_mut() results printable, and it still lists only test_display. Please also state that elements use Debug formatting.
  2. The same comment ("Elements use the Debug format...") is repeated in two tests; the project prefers few comments.
  3. test_display_slice and test_display_slice_mut are near-duplicates; a small shared helper would reduce that.
  4. If the project CHANGELOG lists additive public API, please add an entry for the new Display impls.

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.

@Mec-iS Mec-iS left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks 💯

@Mec-iS
Mec-iS merged commit 1749387 into smartcorelib:main Oct 20, 2026
15 checks passed
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.

Define default formatter for linalg::basic::arrays::{Array, Array2}

2 participants