Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make Display and Debug outputs concise and consistent #1119

Open
wants to merge 16 commits into
base: dev
Choose a base branch
from
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 4 additions & 9 deletions src/base/array_storage.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use std::fmt::{self, Debug, Formatter};
use std::fmt::Debug;
// use std::hash::{Hash, Hasher};
use std::ops::Mul;

Expand All @@ -9,6 +9,8 @@ use serde::ser::SerializeSeq;
#[cfg(feature = "serde-serialize-no-std")]
use serde::{Deserialize, Deserializer, Serialize, Serializer};
#[cfg(feature = "serde-serialize-no-std")]
use std::fmt::{self, Formatter};
#[cfg(feature = "serde-serialize-no-std")]
use std::marker::PhantomData;

use crate::base::allocator::Allocator;
Expand All @@ -26,7 +28,7 @@ use std::mem;
*/
/// A array-based statically sized matrix data storage.
#[repr(transparent)]
#[derive(Copy, Clone, PartialEq, Eq, Hash)]
#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)]
#[cfg_attr(feature = "rkyv-serialize", derive(bytecheck::CheckBytes))]
#[cfg_attr(
feature = "rkyv-serialize-no-std",
Expand Down Expand Up @@ -62,13 +64,6 @@ where
}
}

impl<T: Debug, const R: usize, const C: usize> Debug for ArrayStorage<T, R, C> {
#[inline]
fn fmt(&self, fmt: &mut Formatter<'_>) -> fmt::Result {
self.0.fmt(fmt)
}
}

unsafe impl<T, const R: usize, const C: usize> RawStorage<T, Const<R>, Const<C>>
for ArrayStorage<T, R, C>
{
Expand Down
181 changes: 125 additions & 56 deletions src/base/matrix.rs
Original file line number Diff line number Diff line change
Expand Up @@ -195,9 +195,32 @@ pub struct Matrix<T, R, C, S> {
_phantoms: PhantomData<(T, R, C)>,
}

impl<T, R: Dim, C: Dim, S: fmt::Debug> fmt::Debug for Matrix<T, R, C, S> {
fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> Result<(), fmt::Error> {
self.data.fmt(formatter)
impl<T: fmt::Debug, R: Dim, C: Dim, S: RawStorage<T, R, C> + fmt::Debug> fmt::Debug
for Matrix<T, R, C, S>
{
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> Result<(), fmt::Error> {
if f.alternate() {
writeln!(f, "")?;
}
write!(f, "[")?;

let row_separator = match f.alternate() {
true => ";\n ",
false => "; ",
};

for (i, row) in self.row_iter().enumerate() {
if i > 0 {
write!(f, "{row_separator}")?;
}
for (j, element) in row.iter().enumerate() {
if j > 0 {
write!(f, ", ")?;
}
element.fmt(f)?;
}
}
write!(f, "]")
}
}

Expand Down Expand Up @@ -1855,68 +1878,94 @@ macro_rules! impl_fmt {
S: RawStorage<T, R, C>,
{
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
#[cfg(feature = "std")]
fn val_width<T: Scalar + $trait>(val: &T, f: &mut fmt::Formatter<'_>) -> usize {
match f.precision() {
Some(precision) => format!($fmt_str_with_precision, val, precision)
.chars()
.count(),
None => format!($fmt_str_without_precision, val).chars().count(),
if !f.alternate() {
// pretty print in 2D layout
#[cfg(feature = "std")]
fn val_width<T: Scalar + $trait>(val: &T, f: &mut fmt::Formatter<'_>) -> usize {
match f.precision() {
Some(precision) => format!($fmt_str_with_precision, val, precision)
.chars()
.count(),
None => format!($fmt_str_without_precision, val).chars().count(),
}
}
}

#[cfg(not(feature = "std"))]
fn val_width<T: Scalar + $trait>(_: &T, _: &mut fmt::Formatter<'_>) -> usize {
4
}
#[cfg(not(feature = "std"))]
fn val_width<T: Scalar + $trait>(_: &T, _: &mut fmt::Formatter<'_>) -> usize {
4
}

let (nrows, ncols) = self.shape();
let (nrows, ncols) = self.shape();

if nrows == 0 || ncols == 0 {
return write!(f, "[ ]");
}
if nrows == 0 || ncols == 0 {
return write!(f, "[ ]");
}

let mut max_length = 0;

let mut max_length = 0;
for i in 0..nrows {
for j in 0..ncols {
max_length = crate::max(max_length, val_width(&self[(i, j)], f));
}
}

for i in 0..nrows {
for j in 0..ncols {
max_length = crate::max(max_length, val_width(&self[(i, j)], f));
let max_length_with_space = max_length + 1;

writeln!(f)?; // leading newline to ensure no offset from previous prints
writeln!(
f,
" ┌ {:>width$} ┐",
"",
width = max_length_with_space * ncols - 1
)?;

for i in 0..nrows {
write!(f, " │")?;
for j in 0..ncols {
let number_length = val_width(&self[(i, j)], f) + 1;
let pad = max_length_with_space - number_length;
write!(f, " {:>thepad$}", "", thepad = pad)?;
match f.precision() {
Some(precision) => {
write!(f, $fmt_str_with_precision, (*self)[(i, j)], precision)?
}
None => write!(f, $fmt_str_without_precision, (*self)[(i, j)])?,
}
}
writeln!(f, " │")?;
}
}

let max_length_with_space = max_length + 1;

writeln!(f)?;
writeln!(
f,
" ┌ {:>width$} ┐",
"",
width = max_length_with_space * ncols - 1
)?;

for i in 0..nrows {
write!(f, " │")?;
for j in 0..ncols {
let number_length = val_width(&self[(i, j)], f) + 1;
let pad = max_length_with_space - number_length;
write!(f, " {:>thepad$}", "", thepad = pad)?;
match f.precision() {
Some(precision) => {
write!(f, $fmt_str_with_precision, (*self)[(i, j)], precision)?
write!(
f,
" └ {:>width$} ┘",
"",
width = max_length_with_space * ncols - 1
)
} else {
// print on single line with semicolon-delimited rows and comma-delimited columns
let (nrows, ncols) = self.shape();
write!(f, "[")?;
for row in 0..nrows {
for col in 0..ncols {
if col != 0 {
write!(f, ", ")?;
}
None => write!(f, $fmt_str_without_precision, (*self)[(i, j)])?,
match f.precision() {
Some(precision) => write!(
f,
$fmt_str_with_precision,
(*self)[(row, col)],
precision
)?,
None => write!(f, $fmt_str_without_precision, (*self)[(row, col)])?,
}
}
if row != nrows - 1 {
write!(f, "; ")?;
}
}
writeln!(f, " │")?;
write!(f, "]")
}

writeln!(
f,
" └ {:>width$} ┘",
"",
width = max_length_with_space * ncols - 1
)?;
writeln!(f)
}
}
};
Expand All @@ -1930,6 +1979,28 @@ impl_fmt!(fmt::UpperHex, "{:X}", "{:1$X}");
impl_fmt!(fmt::Binary, "{:b}", "{:.1$b}");
impl_fmt!(fmt::Pointer, "{:p}", "{:.1$p}");

/// Displays a vector using commas as the delimiter
pub fn format_column_vec_as_row<T: Scalar + fmt::Display, D: crate::DimName>(
vector: &OVector<T, D>,
f: &mut fmt::Formatter<'_>,
) -> fmt::Result
where
DefaultAllocator: Allocator<T, D>,
{
if vector.is_empty() {
return write!(f, "[ ]");
}

write!(f, "[")?;
let mut it = vector.iter();
std::fmt::Display::fmt(it.next().unwrap(), f)?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would be nice not to panic on 0-element vectors, unless we're really utterly certain that will never occur (but the signature doesn't suggest as much).

Copy link
Sponsor Collaborator

Choose a reason for hiding this comment

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

0-length vectors can easily occur in practice, so this is important!

Would be nice to have some unit tests that include 0-element vectors maybe..

Copy link
Author

Choose a reason for hiding this comment

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

I've added a test for 0-element matrices, though I'm not sure how to add a test for this function directly. Since calling that function requires a Formatter struct, and I don't think I can construct a 0-length translation/point to test with.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Any reason we're fully-qualifying fmt everywhere rather than just invoking it as a method?

Copy link
Author

Choose a reason for hiding this comment

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

This was to disambiguate it from std::fmt::Debug::fmt.

for comp in it {
write!(f, ", ")?;
std::fmt::Display::fmt(comp, f)?;
}
write!(f, "]")
Comment on lines +1990 to +2001
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: this could be simpler/less unwrappy:

Suggested change
if vector.is_empty() {
return write!(f, "[ ]");
}
write!(f, "[")?;
let mut it = vector.iter();
std::fmt::Display::fmt(it.next().unwrap(), f)?;
for comp in it {
write!(f, ", ")?;
std::fmt::Display::fmt(comp, f)?;
}
write!(f, "]")
write!(f, "[")?;
let mut it = vector.iter();
if let Some(first) = it.next() {
std::fmt::Display::fmt(first, f)?;
for comp in it {
write!(f, ", ")?;
std::fmt::Display::fmt(comp, f)?;
}
}
write!(f, "]")

This also avoids the whitespace in empty vectors, which I think is more consistent.

}

#[cfg(test)]
mod tests {
#[test]
Expand All @@ -1948,9 +2019,7 @@ mod tests {
┌ ┐
│ 1e6 2e5 │
│ 2e-5 1e0 │
└ ┘

"
└ ┘"
)
}
}
Expand Down
25 changes: 6 additions & 19 deletions src/geometry/dual_quaternion.rs
Original file line number Diff line number Diff line change
Expand Up @@ -951,25 +951,12 @@ impl<T: RealField> Default for UnitDualQuaternion<T> {

impl<T: RealField + fmt::Display> fmt::Display for UnitDualQuaternion<T> {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
if let Some(axis) = self.rotation().axis() {
let axis = axis.into_inner();
write!(
f,
"UnitDualQuaternion translation: {} − angle: {} − axis: ({}, {}, {})",
self.translation().vector,
self.rotation().angle(),
axis[0],
axis[1],
axis[2]
)
} else {
write!(
f,
"UnitDualQuaternion translation: {} − angle: {} − axis: (undefined)",
self.translation().vector,
self.rotation().angle()
)
}
write!(f, "{{ translation: ")?;
crate::format_column_vec_as_row(&self.translation().vector, f)?;
write!(f, ", ")?;
write!(f, "rotation: ")?;
std::fmt::Display::fmt(&self.rotation(), f)?;
write!(f, " }}")
}
}

Expand Down
12 changes: 6 additions & 6 deletions src/geometry/isometry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -548,11 +548,11 @@ where
R: fmt::Display,
{
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
let precision = f.precision().unwrap_or(3);

writeln!(f, "Isometry {{")?;
write!(f, "{:.*}", precision, self.translation)?;
write!(f, "{:.*}", precision, self.rotation)?;
writeln!(f, "}}")
write!(f, "{{ translation: ")?;
crate::format_column_vec_as_row(&self.translation.vector, f)?;
write!(f, ", ")?;
write!(f, "rotation: ")?;
std::fmt::Display::fmt(&self.rotation, f)?;
write!(f, " }}")
}
}
22 changes: 12 additions & 10 deletions src/geometry/point.rs
Original file line number Diff line number Diff line change
Expand Up @@ -457,17 +457,19 @@ impl<T: Scalar + fmt::Display, D: DimName> fmt::Display for OPoint<T, D>
where
DefaultAllocator: Allocator<T, D>,
{
/// ```rust
/// # use nalgebra::Point3;
/// let point = Point3::new(1.12345678, 2.12345678, 3.12345678);
/// let rounded = format!("{:#.4}", point); // print point in compact representation
/// assert_eq!(rounded, "[1.1235, 2.1235, 3.1235]");
/// let vertical = format!("{}", point); // print point as a column
/// assert_eq!(vertical, "\n ┌ ┐\n │ 1.12345678 │\n │ 2.12345678 │\n │ 3.12345678 │\n └ ┘");
/// ```
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
write!(f, "{{")?;

let mut it = self.coords.iter();

write!(f, "{}", *it.next().unwrap())?;

for comp in it {
write!(f, ", {}", *comp)?;
if f.alternate() {
crate::format_column_vec_as_row(&self.coords, f)
} else {
std::fmt::Display::fmt(&self.coords, f) // pretty-prints vector
}

write!(f, "}}")
}
}