Make Display and Debug outputs concise and consistent #1119
Conversation
owenbrooks
force-pushed
the
formatting_fixes
branch
from
6d60afc
to
c6b348e
Compare
|
I definitely like the new Display without type names, as well as usage of
|
owenbrooks
force-pushed
the
formatting_fixes
branch
2 times, most recently
from
5df8a77
to
37de790
Compare
|
Thanks for working on this @owenbrooks! I'm very short on time today but I wanted to make some initial comments; I will probably have more later, when I have the time. I'll restrict my comments to the It would be great if we could resolve this while we're at it, as the resolution may impact the other geometric types as well. In short:
To resolve both these issues, I suggest that we use the same syntax that we use for matrix construction - through
Note that this means that vectors will always print as e.g. Finally, I think there should not be a type name for the matrix in the debug output. |
Good point. It was forwarding to Debug for normal but using iterating through the values and using write! instead gets that normal display formatting now.
My feeling is that as a matrix library its nice to see matrices multi-line by default, and these vector-based types are an exception where single line is more convenient, but the alternate form is still there if needed.
True, I've added that now.
Here they are: |
Oh yes, it's something I definitely wanted in the past but wasn't sure if in scope of this PR. Being able to copy-paste debug output into matrix construction would be a big deal. Perhaps we could follow the same for point & vector too. |
|
@RReverser: vectors are matrices, so they'd be printed as e.g. |
|
Sorry, I didn't mean to suggest that you have to resolve #1071 in this PR - we don't have to increase the scope. We could also do it in several stages, as separate PRs, but in any case I think it's worth thinking about to make sure that everything ends up consistent in the end! |
Ah right, that's unfortunate. Ideally |
What you're describing is exactly the behavior of
|
That's fair. I guess the suggestion to output macro form applies only to |
This is reasonable, but IMO consistency is more important. It's really weird for one type to have exactly the opposite behavior of the others. I agree with @Andlon that we should standardize on "alternate form" meaning fancy 2D layout, which is also consistent with the stdlib use of it in |
Wanting consistency between the types does make sense. I would lean towards 2D printing by default for Display as it works currently in nalgebra (and making Point consistent with this). Looking at how printing is handled in projects like numpy, eigen, ndarray, matlab, R, they all print matrices multi line at least by default (though vectors tend to be row vectors and so take up a single line). Since nalgebra is focussed on 2D matrices, printing in 2D feels like a very canonical string representation. It's harder to see 2D patterns when the matrix is squished onto one line. If we make the Debug output semicolon-delimited row-wise would that be usable enough for the case when single-line is desirable? Could do without alternate in that case. Otherwise, could switch it around so that “alternate” forces single line for Display. @Andlon I will undo the addition of "Matrix " to debug output. I will also take a look at fixing that matrix Debug, it may become a separate PR. |
I've said this before, but I'd really want to keep single-line Points for short Display. The fact they're columns in nalgebra feels almost like implementation detail, since in most contexts people would just want to see their coordinates. |
That suits me fine, at least. Having an alternate form mode that's exactly equivalent to
Are your requirements not addressed by just using |
Personally, I think that's fine since semantically they're different types. If anything, I agree that consistency is crucial for Debug formatting, but Display is supposed to be easily human-readable first and foremost, and for that it should have some leeway to write whatever makes most sense in the context. Debug would be fine for a workaround, but we are adding type names consistently to all types, and it would be more verbose than Display. That's expected, Debug is pretty much always meant to be the more verbose option, but it also means we should leverage Display for the more readable option. |
Add format option for concise printing of matrices
as semicolon-delimited rows. e.g. println!("{:#}", matrix);
owenbrooks
force-pushed
the
formatting_fixes
branch
from
37de790
to
49e3633
Compare
|
I've reworked it a bit as follows:
Requires a couple of changes to tests still. This should fix #1071 and #898 New format outputOld output for referenceNew format: static/dynamic matrices, ArrayStorage and VecStorageOld format: static/dynamic matrices, ArrayStorage and VecStorage |
|
This |
|
Perhaps the strongest argument in favor of a readable, concise |
|
Yep that makes a lot of sense. Thanks for your explanation! How is this: |
|
Thanks, I'm much happier with that (and I appreciate your patience working through all these drafts!). I like what you did with the Debug Alternate format too; that's a nice middle ground, and consistent with the spirit of how it's used in general. I am a bit confused as to why the default precision is different between |
Yep, it's just passing through all the formatting options to the underlying float formatter. My understanding is that for floating point Display, when precision is not specified, it tries to create the shortest possible string, omitting the decimal for values like 4.00. It's similar for Debug but the minimum precision is set to 1. See https://doc.rust-lang.org/src/core/fmt/float.rs.html#187 Hmm, I've just realised that this does mean that not everything will be perfectly aligned in debug alternate. My feeling is that is acceptable since Display is handling this. E.g. |
New output |
I think this is fine. People who care can use padding/precision specifiers as usual.
I still personally prefer the more concise |
|
Just a few comments (have more to say but no time atm): I do think alignment in alternate debug mode matters a lot for readability, but I think this can be punted to a separate PR. It's any way a huge improvement over the current (very broken!) state.
For a vector, the order of the components is given. A quaternion has multiple ways in which it can be represented as a vector, say, as |
That's a fair point, although the documentation is clear. How about My opinion here isn't super strong, this PR is a nice improvement as-is. |
owenbrooks
force-pushed
the
formatting_fixes
branch
from
406730f
to
b16a3ed
Compare
src/base/matrix.rs
Outdated
| let nrows = self.nrows(); | ||
| let ncols = self.ncols(); | ||
| for i in 0..nrows { | ||
| for j in 0..ncols { | ||
| if j != 0 { | ||
| write!(f, ", ")?; | ||
| } | ||
| // Safety: the indices are within range | ||
| unsafe { | ||
| (*self.data.get_unchecked(i, j)).fmt(f)?; | ||
| } | ||
| } | ||
| if i != nrows - 1 { | ||
| write!(f, "; ")?; | ||
| } | ||
| if f.alternate() && i != nrows - 1 { | ||
| write!(f, "\n ")?; | ||
| } | ||
| } | ||
| write!(f, "]") |
There was a problem hiding this comment.
Is there a more straightforward / nicer way of looping through in this order?
There was a problem hiding this comment.
Maybe, but I wouldn't overthink it; this seems fine.
There was a problem hiding this comment.
I think it's fine: it's clear what's going on. Is the unsafe really necessary though? It seems to me that the bounds checks here won't lead to a very significant performance penalty given the cost of formatting etc.
There was a problem hiding this comment.
Good point, I've replaced it with self[(i, j)].fmt(f)?;
There was a problem hiding this comment.
I realized that the original code (with unsafe) is in fact unsound for 0-row matrices. It will still panic with your replacement. It's a bit tricky to get this right. Here's a quick attempt I made, which I think should work in all cases?
(btw, would be great to have some unit tests that also cover the 0-dimension corner cases)
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, "]")There was a problem hiding this comment.
Oh, and also be wary of the overflow for evaluating nrows - 1 if nrows == 0!
There was a problem hiding this comment.
Hmm, wouldn't the for loops only run the code if the dimensions are non-zero? for i in 0..0 never runs for instance. Good catch on nrows - 1
There was a problem hiding this comment.
That code looks good to me, I've added it in.
There was a problem hiding this comment.
Hmm, wouldn't the for loops only run the code if the dimensions are non-zero?
for i in 0..0never runs for instance. Good catch on nrows - 1
Yeah, you're right about that! Didn't think that through :-)
src/base/matrix.rs
Outdated
| /// 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>, | ||
| { | ||
| 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, "]") | ||
| } | ||
|
|
There was a problem hiding this comment.
Not sure where this function should go. It is used for the vector-wrapping geometry types.
src/base/matrix.rs
Outdated
| writeln!( | ||
| f, | ||
| " └ {:>width$} ┘", | ||
| "", | ||
| width = max_length_with_space * ncols - 1 | ||
| )?; | ||
| writeln!(f) | ||
| write!( | ||
| f, | ||
| " └ {:>width$} ┘", | ||
| "", | ||
| width = max_length_with_space * ncols - 1 | ||
| ) |
There was a problem hiding this comment.
Previously this added two newlines after the end of the matrix. i.e. [ 1.0 ]/n/n
This removes both of those giving flexibility to write immediately after the matrix. Could add one or both of the newlines back in if we don't want to change it too much.
println!("Display");
println!("{}directly after", smat);
println!("next line");Old:
Display
┌ ┐
│ 1 2 3 4 │
│ 5 6 7 8 │
└ ┘
directly after
next line
New:
Display
┌ ┐
│ 1 2 3 4 │
│ 5 6 7 8 │
└ ┘directly after
next line
There was a problem hiding this comment.
This looks like the right decision to me.
Is the initial newline necessary though? I guess it is in order to give reasonable formatting for cases where you have e.g.
println!("My matrix is {matrix}");I guess if you don't have a leading newline, cases like this will end up with messed up formatting?
There was a problem hiding this comment.
Yeah I assume that is why the initial newline is included. I wouldn't be against removing it myself.
| { | ||
| write!(f, "[")?; | ||
| let mut it = vector.iter(); | ||
| std::fmt::Display::fmt(it.next().unwrap(), f)?; |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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..
There was a problem hiding this comment.
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.
| { | ||
| write!(f, "[")?; | ||
| let mut it = vector.iter(); | ||
| std::fmt::Display::fmt(it.next().unwrap(), f)?; |
There was a problem hiding this comment.
Any reason we're fully-qualifying fmt everywhere rather than just invoking it as a method?
There was a problem hiding this comment.
This was to disambiguate it from std::fmt::Debug::fmt.
Hmm, you're right that the docs are pretty clear on the storage order. I generally agree with you that conciseness is really important, given that during debugging you might be outputting complex structs or large amounts of data, and verbose output is a real hindrance to effective debugging in my experience. So I think I'd also be in favor of the more compact 4-number output for quaternions. |
| 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, "]") |
There was a problem hiding this comment.
nit: this could be simpler/less unwrappy:
| 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.
|
@owenbrooks: do you intend to continue working on this issue? I think your contribution here is very valuable and I think we were close to having converged. Would be nice to wrap it up if we can, although of course I understand if other things take priority for you. |
Summary
Differences when printing (expand)