Skip to content

Commit

Permalink
Increase "Design Water Flow Rate [m3/s]" for CT to 6 digits
Browse files Browse the repository at this point in the history
  • Loading branch information
@jmarrec
jmarrec committed Jan 21, 2025
1 parent 6265d96 commit c2fee76
Show file tree
Hide file tree
Showing 2 changed files with 2 additions and 2 deletions.
2 changes: 1 addition & 1 deletion src/EnergyPlus/CondenserLoopTowers.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3380,7 +3380,7 @@ namespace CondenserLoopTowers {
state, state.dataOutRptPredefined->pdchCTFCDesFanPwr, this->Name, this->HighSpeedFanPower); // equivalent to Design Fan Power?
OutputReportPredefined::PreDefTableEntry(state, state.dataOutRptPredefined->pdchCTFCDesInletAirWBT, this->Name, this->DesignInletWB);
OutputReportPredefined::PreDefTableEntry(
state, state.dataOutRptPredefined->pdchCTFCDesWaterFlowRate, this->Name, this->DesignWaterFlowRate);
state, state.dataOutRptPredefined->pdchCTFCDesWaterFlowRate, this->Name, this->DesignWaterFlowRate, 6);
OutputReportPredefined::PreDefTableEntry(state, state.dataOutRptPredefined->pdchCTFCLevWaterSPTemp, this->Name, this->DesOutletWaterTemp);
}

Expand Down
2 changes: 1 addition & 1 deletion tst/EnergyPlus/unit/CondenserLoopTowers.unit.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4951,7 +4951,7 @@ TEST_F(EnergyPlusFixture, CondenserLoopTowers_VSCoolingTower_OutputReport)
EXPECT_EQ("1000.00", OutputReportPredefined::RetrievePreDefTableEntry(*state, orp.pdchCTFCDesFanPwr, TowerName));
EXPECT_EQ(fmt::format("{:.2f}", expectedDesignInletWB),
OutputReportPredefined::RetrievePreDefTableEntry(*state, orp.pdchCTFCDesInletAirWBT, TowerName));
EXPECT_EQ("0.02", OutputReportPredefined::RetrievePreDefTableEntry(*state, orp.pdchCTFCDesWaterFlowRate, TowerName));
EXPECT_EQ("0.020000", OutputReportPredefined::RetrievePreDefTableEntry(*state, orp.pdchCTFCDesWaterFlowRate, TowerName));
EXPECT_EQ(fmt::format("{:.2f}", expectedDesOutletWaterTemp),
OutputReportPredefined::RetrievePreDefTableEntry(*state, orp.pdchCTFCLevWaterSPTemp, TowerName));
EXPECT_EQ("COOLINGTOWER LOOP", OutputReportPredefined::RetrievePreDefTableEntry(*state, orp.pdchCTFCCondLoopName, TowerName));
Expand Down

11 comments on commit c2fee76

@nrel-bot-2
Copy link

Choose a reason for hiding this comment

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

10889_CoolingTower_ReportWBT (jmarrec) - x86_64-Linux-Ubuntu-24.04-gcc-13.3: OK (2921 of 2921 tests passed, 0 test warnings)

Build Badge Test Badge

@nrel-bot-2
Copy link

Choose a reason for hiding this comment

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

10889_CoolingTower_ReportWBT (jmarrec) - x86_64-Linux-Ubuntu-24.04-gcc-13.3-UnitTestsCoverage-RelWithDebInfo: OK (2103 of 2103 tests passed, 0 test warnings)

Build Badge Test Badge Coverage Badge

@nrel-bot-2
Copy link

Choose a reason for hiding this comment

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

10889_CoolingTower_ReportWBT (jmarrec) - x86_64-Linux-Ubuntu-24.04-gcc-13.3-IntegrationCoverage-RelWithDebInfo: OK (801 of 801 tests passed, 0 test warnings)

Build Badge Test Badge Coverage Badge

@amirroth
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are six digits necessary? Maybe two is not sufficient, but why six?

There is actually a bigger question behind this one which is that I believe that we should move to 32-bit floating-point for performance reasons and I am wondering how to inoculate our outputs and testing against that eventual move. FWIW 32-bit floating-point is good to 7 decimal places, 64-bit is good to 16 decimal places which in my opinion is an overkill for BEM, where only Pi is known to that level of precision.

@jmarrec
Copy link
Contributor Author

@jmarrec jmarrec commented on c2fee76 Jan 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Cause I found this.

EnergyPlus/src/EnergyPlus/Pumps.cc

Line 2270 in 31e3c33

PreDefTableEntry(state, thisReport->pdchPumpFlow, equipName, thisPump.NomVolFlowRate, 6);

6 sig digits gives a precision of 0.01 GPM (gal/min)

@jmarrec
Copy link
Contributor Author

Choose a reason for hiding this comment

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

EnergyPlus/src/EnergyPlus/ReportCoilSelection.cc

Lines 216 to 217 in 31e3c33

OutputReportPredefined::PreDefTableEntry(
state, state.dataOutRptPredefined->pdchCoilFinalPlantVolFlowRate, c->coilName_, c->coilRefWaterVolFlowFinal, 8);

EnergyPlus/src/EnergyPlus/ReportCoilSelection.cc

Lines 380 to 381 in 31e3c33

OutputReportPredefined::PreDefTableEntry(
state, state.dataOutRptPredefined->pdchCoilFlowPrcntPlantFlow, c->coilName_, c->coilFlowPrcntPlantFlow, 6);

@jmarrec
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'm fine changing it to 4, or whatever you say though.

@amirroth
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, any thoughts about the larger issue? Maybe we can take that offline.

@amirroth
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm pretty sure that the precision corresponds to digits after the decimal point, not in the number as a whole.

@jmarrec
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

EnergyPlus/src/EnergyPlus/OutputReportPredefined.cc

Lines 1613 to 1634 in 31e3c33

int sigDigitCount = 2;
// check for number of significant digits
if (present(numSigDigits)) {
if ((numSigDigits <= 9) && (numSigDigits >= 0)) {
sigDigitCount = numSigDigits;
}
}
if (std::abs(tableEntryReal) < 1e8) { // change from 1e10 for more robust entry writing
// something changed in FMT 7.x and "{:#12.{}F}" now outputs 13. So changing it to 11.{}F to maintain existing functionality. Likely
// related to https://github.com/fmtlib/fmt/issues/1893
state.dataOutRptPredefined->tableEntry(state.dataOutRptPredefined->numTableEntry).charEntry =
format("{:#11.{}F}", tableEntryReal, sigDigitCount);
} else {
// Formatting in scientific notation, zero sigDigits makes zero sense.
// **for something greater than 1E+08**, one sigDigits is very unhelpful (you're having an accuracy of 0.5E+07 at best)
if (sigDigitCount < 2) {
sigDigitCount = 2;
}
state.dataOutRptPredefined->tableEntry(state.dataOutRptPredefined->numTableEntry).charEntry =
format("{:12.{}Z}", tableEntryReal, sigDigitCount);
}

@jmarrec
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is actually a bigger question behind this one which is that I believe that we should move to 32-bit floating-point for performance reasons and I am wondering how to inoculate our outputs and testing against that eventual move. FWIW 32-bit floating-point is good to 7 decimal places, 64-bit is good to 16 decimal places which in my opinion is an overkill for BEM, where only Pi is known to that level of precision.

You mean std::float32_t from C++23 here? or just "float"? https://en.cppreference.com/w/cpp/types/floating-point

Have you already tried changing the typedef and measuring performance? Is there really a performance gain?

EnergyPlus/src/EnergyPlus/api/TypeDefs.h

Lines 51 to 52 in 31e3c33

typedef float Real32; // Platform-specific: C++ has no defined precision floating point types
typedef double Real64; // Platform-specific: C++ has no defined precision floating point types

Please sign in to comment.