[trivial] restrict format changes to Logging. #199

SimonDold · 2023-12-03T13:31:30Z

No description provided.

jendrikseipp

Thanks for taking care of this! I would even go one step further and only set the fixed precision for when a timer is printed by a logger, because only there do we want a fixed length of the output. In other places in the code, at least I would prefer a timer output of e.g., "1.01s" to "1.010000s".

silvansievers · 2023-12-04T07:08:34Z

src/search/utils/logging.cc

@@ -133,4 +133,18 @@ static plugins::TypedEnumPlugin<Verbosity> _enum_plugin({
        {"verbose", "full output"},
        {"debug", "like verbose with additional debug output"}
    });
+
+void Log::prepend_header(ostream &stream) {


With saving, changing and undoing the precision flags, this method does a lot of work for what used to be a simple printing. Could this possibly slow down the overall runtime to an extent we don't want?

I hope that the runtime for this is negligible compared to measuring the time and the peak memory usage. But we should run an experiment with blind search that confirms this.

jendrikseipp · 2023-12-04T08:09:19Z

src/search/utils/logging.cc

@@ -133,4 +133,18 @@ static plugins::TypedEnumPlugin<Verbosity> _enum_plugin({
        {"verbose", "full output"},
        {"debug", "like verbose with additional debug output"}
    });
+
+void Log::prepend_header(ostream &stream) {


Suggested change

void Log::prepend_header(ostream &stream) {

void Log::add_prefix(ostream &stream) {

Getting a setting precision should have trivial runtime compared to the actual output. The output has to do a floating-point division for every digit. Getting/setting precision is just returning/setting some ints. I would not run an experiment.

jendrikseipp · 2023-12-04T08:10:02Z

src/search/utils/logging.cc

+
+    stream.flags(previous_flags); // undo format flag changes
+
+    stream << ", "


Remove line break.

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.

Shouldn't this be a const function?

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

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 │ └──────────┴────────────┴──────────────────────────┘```

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.

jendrikseipp · 2023-12-04T08:11:24Z

src/search/utils/logging.h

@@ -34,7 +34,9 @@ enum class Verbosity {
 class Log {
    std::ostream &stream;
    const Verbosity verbosity;
+    static const int output_precision = 6;


Suggested change

static const int output_precision = 6;

static const int TIMER_PRECISION = 6;

jendrikseipp · 2023-12-04T08:12:30Z

src/search/utils/logging.cc

+
+    stream << ", "
+           << get_peak_memory_in_kb() << " KB] ";
+}


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

jendrikseipp · 2023-12-04T08:13:48Z

src/search/utils/logging.h

@@ -34,7 +34,9 @@ enum class Verbosity {
 class Log {
    std::ostream &stream;
    const Verbosity verbosity;
+    static const int output_precision = 6;


Move the static member above all instance members.

maltehelmert · 2023-12-04T11:22:35Z

Thanks for taking care of this! I would even go one step further and only set the fixed precision for when a timer is printed by a logger, because only there do we want a fixed length of the output. In other places in the code, at least I would prefer a timer output of e.g., "1.01s" to "1.010000s".

Where do you see 1.010000s? I thought we used the high-precision timer everywhere in the C++ code these days.

SimonDold added 5 commits November 24, 2023 00:46

[trivial] fix timer precision.

cdc4ab2

improve style.

115ffee

reduce precision to match Lab.

e2157d9

set precision to 6 digits.

3febdde

restrict format changes to Duration.

99d7228

jendrikseipp requested changes Dec 3, 2023

View reviewed changes

SimonDold added 3 commits December 3, 2023 20:04

restrict format changes to Logging.

015f0ef

remove unused include.

ed92f1b

fix style.

a7cf212

SimonDold changed the title ~~[trivial] restrict format changes to Duration.~~ [trivial] restrict format changes to Logging. Dec 3, 2023

silvansievers reviewed Dec 4, 2023

View reviewed changes

jendrikseipp requested changes Dec 4, 2023

View reviewed changes

SimonDold added 2 commits December 4, 2023 15:39

handling feedback from code review.

3ac9680

return previous precision.

2f5a6a7

SimonDold merged commit 9c5107f into aibasel:main Dec 18, 2023
6 of 11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[trivial] restrict format changes to Logging. #199

[trivial] restrict format changes to Logging. #199

SimonDold commented Dec 3, 2023

jendrikseipp left a comment

silvansievers Dec 4, 2023

jendrikseipp Dec 4, 2023

jendrikseipp Dec 4, 2023

maltehelmert Dec 4, 2023

jendrikseipp Dec 4, 2023

maltehelmert Dec 4, 2023 •

edited

Loading

maltehelmert Dec 4, 2023

SimonDold Dec 4, 2023

SimonDold Dec 4, 2023

maltehelmert Dec 4, 2023

jendrikseipp Dec 4, 2023

jendrikseipp Dec 4, 2023

jendrikseipp Dec 4, 2023

maltehelmert commented Dec 4, 2023

	void Log::prepend_header(ostream &stream) {
	void Log::add_prefix(ostream &stream) {


		stream.flags(previous_flags); // undo format flag changes

		stream << ", "

	static const int output_precision = 6;
	static const int TIMER_PRECISION = 6;

[trivial] restrict format changes to Logging. #199

[trivial] restrict format changes to Logging. #199

Conversation

SimonDold commented Dec 3, 2023

jendrikseipp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maltehelmert Dec 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maltehelmert commented Dec 4, 2023

maltehelmert Dec 4, 2023 •

edited

Loading