#9000 - Average values for integer output variables fail debug assert and are rounded to 1 place #9294

jmarrec · 2022-02-22T16:42:35Z

Pull request overview

Fixes Average values for integer output variables fail debug assert and are rounded to 2 places #9000
The debug assert is because trim was used instead of strip
a number between -1 and 1 would be formatted in WriteReportIntegerData to "{:N}" which sets precision to 1 (and prepend "0." to it)

Pull Request Author

Add to this list or remove from it as applicable. This is a simple templated set of guidelines.

Reviewer

This will not be exhaustively relevant to every PR.

…Out) not trim(NumberOut) cf https://github.com/NREL/EnergyPlus/blob/baff08990c9c4d12d8bbde0378073018a3ea6acf/src/EnergyPlus/OutputProcessor.cc#L5290 5dee4a2#diff-bd9d5281a7fe99bacbcd3d7112772f7804d8e091f10a56a0954a7639c24384fcR4946

…default to 4

…et fmt default it

Myoldmopar · 2022-02-22T20:10:45Z

This is definitely an improvement, and although we can have a broader discussion about the output significance, this one issue is not widespread in any sense of the word, and we should just take this cleanup as soon as it's ready.

jmarrec · 2022-02-22T20:47:17Z

src/EnergyPlus/IOFiles.hh

+                // We'll be formating a number between -1 and 1 as scientific notation, so we want some amount of decimals to be meaningful
+                if (specs_.precision == -1) {
+                    specs_.precision = 4;
+                }


In the N case, default to precision = 4.

I suppose if we were fine with having around 6 precision, we could delete that block and let fmt default it in the Z case which it forwards to.

Yeah I wouldn't care either way, but I think it's reasonable, and we don't need to belabor this corner case.

Is this the default precision for the Fortran output? This whole code is to emulate Fortran output styles within the fmt library.

jmarrec · 2022-02-22T20:47:36Z

src/EnergyPlus/IOFiles.hh

+            } else if (specs_.precision != -1) {
                // We need AT LEAST one in precision so we capture a '.' below
+                // If specs_.precision is -1, we don't need to do anything, keep the fmt defaults which are sane


In the Z case, if the precision wasn't set already, then let fmt default it.

Sounds reasonable, if we want control then we just need to be explicit about the precision we want.

Is this the default precision for the Fortran output? This whole code is to emulate Fortran output styles within the fmt library.

jmarrec · 2022-02-22T20:49:13Z

src/EnergyPlus/OutputProcessor.cc

@@ -4644,7 +4644,7 @@ namespace OutputProcessor {
            NumberOut = "0.0";
        } else {
            NumberOut = format("{:N}", repVal);
-            strip_trailing_zeros(trim(NumberOut));
+            strip_trailing_zeros(strip(NumberOut));


Actual crash. v9.3.0 uses strip. Then during the move away from gio::write, it was first removed then added back as trim instead of strip

cf

EnergyPlus/src/EnergyPlus/OutputProcessor.cc

Line 5290 in baff089

strip_trailing_zeros(strip(NumberOut));

5dee4a2#diff-bd9d5281a7fe99bacbcd3d7112772f7804d8e091f10a56a0954a7639c24384fcR4946

jmarrec · 2022-02-22T20:49:33Z

tst/EnergyPlus/unit/OutputFiles.unit.cc

+    // Tested with E+ 9.3.0
+    EXPECT_EQ(format("{:N}", 0.03576388888888889), "          0.3576E-01");
+    EXPECT_EQ(format("{:.3N}", 0.03576388888888889), "0.358E-01");
+    EXPECT_EQ(format("{:.4N}", 0.03576388888888889), "0.3576E-01");
+


New format test

It says in the comments "Tested with E+ 9.3.0", does that mean NumberOut below will be "0.3576E-01"?

ObjexxFCL::gio::Fmt fmtLD("*"); std::string NumberOut; ObjexxFCL::gio::write(NumberOut, fmtLD) << repVal; strip_trailing_zeros(strip(NumberOut));

jmarrec · 2022-02-22T20:49:56Z

tst/EnergyPlus/unit/OutputProcessor.unit.cc

+        WriteReportIntegerData(*state, 1, "1", 25.75, StoreType::Averaged, 720, ReportingFrequency::Monthly, 0, 4010115, 1, 4011560);
+        EXPECT_TRUE(compare_eso_stream(delimited_string({"1,0.3576E-01,0, 1, 1,15,1, 1,15,60"}, "\n")));


This triggered the assert throw in debug mode before.

Myoldmopar · 2022-02-22T21:31:26Z

src/EnergyPlus/IOFiles.hh

+            } else if (specs_.precision != -1) {
                // We need AT LEAST one in precision so we capture a '.' below
+                // If specs_.precision is -1, we don't need to do anything, keep the fmt defaults which are sane


Sounds reasonable, if we want control then we just need to be explicit about the precision we want.

Myoldmopar · 2022-02-22T21:31:47Z

src/EnergyPlus/IOFiles.hh

+                // We'll be formating a number between -1 and 1 as scientific notation, so we want some amount of decimals to be meaningful
+                if (specs_.precision == -1) {
+                    specs_.precision = 4;
+                }


Yeah I wouldn't care either way, but I think it's reasonable, and we don't need to belabor this corner case.

Myoldmopar · 2022-02-22T21:32:00Z

src/EnergyPlus/OutputProcessor.cc

@@ -4644,7 +4644,7 @@ namespace OutputProcessor {
            NumberOut = "0.0";
        } else {
            NumberOut = format("{:N}", repVal);
-            strip_trailing_zeros(trim(NumberOut));
+            strip_trailing_zeros(strip(NumberOut));


Myoldmopar · 2022-02-22T21:33:16Z

I'll go ahead and let the integration debug build finish but assuming it is fine then I'll merge this. Thanks for the fix @jmarrec and the unit test and walkthrough.

mbadams5

My overall comment is to make sure these changes are inline with Fortran output style. Since I said in my individual comments that this code is solely to emulate Fortran output within the fmt library. Turner originally wrote this code when he converted the GIO output to FMT.

If NumberOut should have a different precision or output style than Fortran, it might be better to just change it to a true fmt::format("Foo", repVal); instead of using this shim.

mbadams5 · 2022-02-22T21:37:26Z

tst/EnergyPlus/unit/OutputFiles.unit.cc

+    // Tested with E+ 9.3.0
+    EXPECT_EQ(format("{:N}", 0.03576388888888889), "          0.3576E-01");
+    EXPECT_EQ(format("{:.3N}", 0.03576388888888889), "0.358E-01");
+    EXPECT_EQ(format("{:.4N}", 0.03576388888888889), "0.3576E-01");
+


It says in the comments "Tested with E+ 9.3.0", does that mean NumberOut below will be "0.3576E-01"?

ObjexxFCL::gio::Fmt fmtLD("*"); std::string NumberOut; ObjexxFCL::gio::write(NumberOut, fmtLD) << repVal; strip_trailing_zeros(strip(NumberOut));

mbadams5 · 2022-02-22T21:39:23Z

src/EnergyPlus/IOFiles.hh

+                // We'll be formating a number between -1 and 1 as scientific notation, so we want some amount of decimals to be meaningful
+                if (specs_.precision == -1) {
+                    specs_.precision = 4;
+                }


Is this the default precision for the Fortran output? This whole code is to emulate Fortran output styles within the fmt library.

mbadams5 · 2022-02-22T21:39:47Z

src/EnergyPlus/IOFiles.hh

+            } else if (specs_.precision != -1) {
                // We need AT LEAST one in precision so we capture a '.' below
+                // If specs_.precision is -1, we don't need to do anything, keep the fmt defaults which are sane


Is this the default precision for the Fortran output? This whole code is to emulate Fortran output styles within the fmt library.

Myoldmopar · 2022-02-22T21:46:24Z

My overall comment is to make sure these changes are inline with Fortran output style.

I hope that's no longer the guiding principle. I would just say let's make sensible output, not try too hard to match Fortran.

mbadams5 · 2022-02-22T21:51:05Z

I don't think we should write new code using the Fortran syntax either. Which is why I suggested rewriting NumberOut = format("{:N}", repVal); to use true FMT syntax NumberOut = fmt::format("{:something.something}", repVal); instead of changing the Fortran shim since we have good coverage of output changes but obviously not complete given the issue this PR is solving.

mjwitte · 2022-03-15T22:18:25Z

Looks like @Myoldmopar approved this and was ready to merge, but then @mbadams5 made some further comments.
Is this ready or waiting on further changes?

… to 1, let fmt default it" This reverts commit 912fd06.

… given: default to 4" This reverts commit 7031e07.

…tegerData

Myoldmopar · 2022-03-21T15:35:58Z

OK, so in an effort to get this merged in, I reverted out the two latter commits which modified the output reporting. This PR now just includes the unit test demonstrating the issue (that initially fails) plus the tiny code change to get the unit test passing. We can have discussions about how to handle the other changes later, but I'd like to get this defect fixed at least.

I then pulled develop in, ran tests, including regressions, and it was entirely clean. I'm inclined to wait for CI to verify that for me since I ran just one release build on just Linux. I'll move on to other PRs while CI catches up here and go from there.

Myoldmopar · 2022-03-21T17:55:48Z

Looking good so far, I'll let the unit test debug build run, and I'll let the debug integration tests at least get started before merging.

Myoldmopar · 2022-03-21T21:25:18Z

No issues here, merging this in. Won't cause any diffs for the rainfall PR which is staged for next merge.

jmarrec added 2 commits February 22, 2022 14:34

Minimal unit test for #9000 that triggers a segfault

5cad312

jmarrec added the Defect Includes code to repair a defect in EnergyPlus label Feb 22, 2022

jmarrec self-assigned this Feb 22, 2022

jmarrec added 2 commits February 22, 2022 17:43

When formatting as N, and number is [-1, 1], and no precision given: …

7031e07

…default to 4

When formatting as Z, and no precision is given, do not force to 1, l…

912fd06

…et fmt default it

jmarrec added the NotIDDChange Code does not impact IDD (can be merged after IO freeze) label Feb 22, 2022

jmarrec commented Feb 22, 2022

View reviewed changes

jmarrec requested a review from Myoldmopar February 22, 2022 20:50

Myoldmopar approved these changes Feb 22, 2022

View reviewed changes

mbadams5 reviewed Feb 22, 2022

View reviewed changes

Myoldmopar added this to the EnergyPlus 22.1 milestone Feb 28, 2022

Myoldmopar added 3 commits March 21, 2022 10:09

Revert "When formatting as Z, and no precision is given, do not force…

9bcf0d0

… to 1, let fmt default it" This reverts commit 912fd06.

Revert "When formatting as N, and number is [-1, 1], and no precision…

16f4d7b

… given: default to 4" This reverts commit 7031e07.

Merge remote-tracking branch 'origin/develop' into 9000_WriteReportIn…

ff4508c

…tegerData

Myoldmopar merged commit d016c0a into develop Mar 21, 2022

Myoldmopar deleted the 9000_WriteReportIntegerData branch March 21, 2022 21:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#9000 - Average values for integer output variables fail debug assert and are rounded to 1 place #9294

#9000 - Average values for integer output variables fail debug assert and are rounded to 1 place #9294

jmarrec commented Feb 22, 2022

Myoldmopar commented Feb 22, 2022

jmarrec Feb 22, 2022

jmarrec Feb 22, 2022

Myoldmopar Feb 22, 2022

mbadams5 Feb 22, 2022

jmarrec Feb 22, 2022

Myoldmopar Feb 22, 2022

mbadams5 Feb 22, 2022

jmarrec Feb 22, 2022

Myoldmopar Feb 22, 2022

jmarrec Feb 22, 2022

mbadams5 Feb 22, 2022

jmarrec Feb 22, 2022

Myoldmopar Feb 22, 2022

Myoldmopar Feb 22, 2022

Myoldmopar Feb 22, 2022

Myoldmopar commented Feb 22, 2022

mbadams5 left a comment

mbadams5 Feb 22, 2022

mbadams5 Feb 22, 2022

mbadams5 Feb 22, 2022

Myoldmopar commented Feb 22, 2022

mbadams5 commented Feb 22, 2022

mjwitte commented Mar 15, 2022

Myoldmopar commented Mar 21, 2022

Myoldmopar commented Mar 21, 2022

Myoldmopar commented Mar 21, 2022

		WriteReportIntegerData(*state, 1, "1", 25.75, StoreType::Averaged, 720, ReportingFrequency::Monthly, 0, 4010115, 1, 4011560);
		EXPECT_TRUE(compare_eso_stream(delimited_string({"1,0.3576E-01,0, 1, 1,15,1, 1,15,60"}, "\n")));

#9000 - Average values for integer output variables fail debug assert and are rounded to 1 place #9294

#9000 - Average values for integer output variables fail debug assert and are rounded to 1 place #9294

Conversation

jmarrec commented Feb 22, 2022

Pull request overview

Pull Request Author

Reviewer

Myoldmopar commented Feb 22, 2022

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

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

Myoldmopar commented Feb 22, 2022

mbadams5 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

Myoldmopar commented Feb 22, 2022

mbadams5 commented Feb 22, 2022

mjwitte commented Mar 15, 2022

Myoldmopar commented Mar 21, 2022

Myoldmopar commented Mar 21, 2022

Myoldmopar commented Mar 21, 2022