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.

Sign up for GitHub

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

[trivial] restrict format changes to Logging. #199

Merged
merged 10 commits into from
Dec 18, 2023
Merged

[trivial] restrict format changes to Logging. #199

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
cdc4ab2
[trivial] fix timer precision.
SimonDold Nov 23, 2023
115ffee
improve style.
SimonDold Nov 24, 2023
e2157d9
reduce precision to match Lab.
SimonDold Nov 24, 2023
3febdde
set precision to 6 digits.
SimonDold Nov 27, 2023
99d7228
restrict format changes to Duration.
SimonDold Dec 3, 2023
015f0ef
restrict format changes to Logging.
SimonDold Dec 3, 2023
ed92f1b
remove unused include.
SimonDold Dec 3, 2023
a7cf212
fix style.
SimonDold Dec 3, 2023
3ac9680
handling feedback from code review.
SimonDold Dec 4, 2023
2f5a6a7
return previous precision.
SimonDold Dec 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions src/search/utils/logging.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,4 +133,16 @@ static plugins::TypedEnumPlugin<Verbosity> _enum_plugin({
{"verbose", "full output"},
{"debug", "like verbose with additional debug output"}
});

void Log::add_prefix() const {
stream << "[t=";
streamsize previous_precision = cout.precision(TIMER_PRECISION);
ios_base::fmtflags previous_flags = stream.flags();
stream.setf(ios_base::fixed, ios_base::floatfield);
stream << g_timer;
stream.flags(previous_flags);
cout.precision(previous_precision);
stream << ", "
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove line break.

Copy link
Contributor

@maltehelmert maltehelmert Dec 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

If it's just for precision, I think a much more idiomatic way to do this is something like (untested):

streamsize old_precision = stream.precision(new_precision);
// do output
stream.precision(old_precision);

Regarding use of fixed, do we not want this throughout the planner anyway and perhaps should do it in initialization? I'm not so clear what it does exactly and which other kinds of outputs we do that may/may not want it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be a const function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes this should be const, and have a signature without parameter, because the stream is already a field of this object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do not think that we want fixed throughout the planner. It would change things like
[t=0.XXXXXXs, 45080 KB] Int hash set load factor: 108/128 = 0.84375
to
[t=0.XXXXXXs, 45080 KB] Int hash set load factor: 108/128 = 0.843750
for blocks/probBLOCKS-4-0.pddl --search "astar(blind())"
(adding tailing zeros).

https://cplusplus.com/reference/ios/fixed/ :
"Notice that the treatment of the precision field differs between the default floating-point notation and the fixed and scientific notations (see precision). On the default floating-point notation, the precision field specifies the maximum number of meaningful digits to display both before and after the decimal point, while in both the fixed and scientific notations, the precision field specifies exactly how many digits to display after the decimal point, even if they are trailing decimal zeros."

https://en.cppreference.com/w/cpp/io/manip/fixed :

│  number  │   iomanip  │      representation      │
├──────────┼────────────┼──────────────────────────┤
│ 0.0      │ fixed      │ 0.000000                 │
│ 0.0      │ scientific │ 0.000000e+00             │
│ 0.0      │ hexfloat   │ 0x0p+0                   │
│ 0.0      │ default    │ 0                        │
├──────────┼────────────┼──────────────────────────┤
│ 0.01     │ fixed      │ 0.010000                 │
│ 0.01     │ scientific │ 1.000000e-02             │
│ 0.01     │ hexfloat   │ 0x1.47ae147ae147bp-7     │
│ 0.01     │ default    │ 0.01                     │
├──────────┼────────────┼──────────────────────────┤
│ 0.00001  │ fixed      │ 0.000010                 │
│ 0.00001  │ scientific │ 1.000000e-05             │
│ 0.00001  │ hexfloat   │ 0x1.4f8b588e368f1p-17    │
│ 0.00001  │ default    │ 1e-05                    │
└──────────┴────────────┴──────────────────────────┘```

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks! Somehow, none of the four options for 0.00001 looks good to me (for example for the hash table output you mention). All of them other than "fixed" are likely to break at least some tools that parse the output, and the trailing zeros for fixed are ugly. I guess the only reason this doesn't currently break is because these are either not commonly parsed, or the numbers are never so close to 0. It's a bit funny that they write it as "0.00001" in the first column of the table, but none of the available options would allow actually displaying it as "0.00001". ;-)

In any case, if we don't want to invest too much time into this, the less invasive way to do this indeed would be to store and restore all flags including "fixed", like the patch currently does. Not because I think the default is better than fixed for things like the hash table example but because it's a less invasive change. Whether or not we want to eventually change these outputs is then perhaps a concern for another time.

<< get_peak_memory_in_kb() << " KB] ";
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd remove the empty lines and the comments since they don't add info IMO.

}
6 changes: 3 additions & 3 deletions src/search/utils/logging.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,11 @@ enum class Verbosity {
Internal class encapsulated by LogProxy.
*/
class Log {
static const int TIMER_PRECISION = 6;
std::ostream &stream;
const Verbosity verbosity;
bool line_has_started;
void add_prefix() const;

public:
explicit Log(Verbosity verbosity)
Expand All @@ -45,10 +47,8 @@ class Log {
Log &operator<<(const T &elem) {
if (!line_has_started) {
line_has_started = true;
stream << "[t=" << g_timer << ", "
<< get_peak_memory_in_kb() << " KB] ";
add_prefix();
}

stream << elem;
return *this;
}
Expand Down
Loading