qa_utils: print errors with enough precision to represent tolerance#860
qa_utils: print errors with enough precision to represent tolerance#860marcusmueller wants to merge 1 commit intomainfrom
Conversation
Signed-off-by: Marcus Müller <mmueller@gnuradio.org>
jdemel
left a comment
jdemel
left a comment
There was a problem hiding this comment.
Thanks a lot. Only very minor comments.
|
|
||
| #include <fmt/core.h> | ||
| #include <fmt/format.h> | ||
| #include <fmt/ranges.h> |
There was a problem hiding this comment.
This is a double include. I suggest to remove the empty lines between includes, let clang-format do the sorting and automatically remove duplicates =D
| float tol, | ||
| int max_errors = 10) | ||
| { | ||
| constexpr std::array<const char*, 5> columns{ |
There was a problem hiding this comment.
Are we able to use std::string_view in place of char* here?
| template <class T> | ||
| unsigned int max_col_length(const T& columns) |
There was a problem hiding this comment.
could you add a new line between functions? =D
could you rename the function to max_column_width? That's what is essentially computed, correct?
| unsigned int index_len = fmt::formatted_size("{}", columns[0]) + 1; | ||
| unsigned int col_len = max_col_length(columns); | ||
| /* choose enough digits to represent tolerance */ | ||
| const auto prec_tup = tol_precision(tol, col_len); |
There was a problem hiding this comment.
I suggest to use structured binding here. Something like:
| const auto prec_tup = tol_precision(tol, col_len); | |
| const auto [tol_length, value_prec] = tol_precision(tol, col_len); |
This would improve readability here a lot. I was looking for the exact meaning of these variables for a while.
| if (fail_indices.empty()) | ||
| return; |
There was a problem hiding this comment.
Could you do me a favor and add curly braces here?
2 participants
Issues like #859 highlight that we're not seeing the numerical values to a precision good enough to debug. Change that.