Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix #8066 - Coil:Cooling:DX:CurveFit:OperatingMode: Nominal Evaporative Condenser Pump Power is marked autosizabled, but not autosized, and other autosized missing units #8247

Merged
merged 9 commits into from
Sep 16, 2020

Conversation

jmarrec
Copy link
Contributor

@jmarrec jmarrec commented Sep 2, 2020

Pull request overview

Pull Request Author

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

  • Title of PR should be user-synopsis style (clearly understandable in a standalone changelog context)
  • Label the PR with at least one of: Defect, Refactoring, NewFeature, Performance, and/or DoNoPublish
  • Pull requests that impact EnergyPlus code must also include unit tests to cover enhancement or defect repair
  • Author should provide a "walkthrough" of relevant code changes using a GitHub code review comment process
  • If any diffs are expected, author must demonstrate they are justified using plots and descriptions
  • If changes fix a defect, the fix should be demonstrated in plots and descriptions
  • If any defect files are updated to a more recent version, upload new versions here or on DevSupport
  • If IDD requires transition, transition source, rules, ExpandObjects, and IDFs must be updated, and add IDDChange label
  • If structural output changes, add to output rules file and add OutputChange label
  • If adding/removing any LaTeX docs or figures, update that document's CMakeLists file dependencies

Reviewer

This will not be exhaustively relevant to every PR.

  • Perform a Code Review on GitHub
  • If branch is behind develop, merge develop and build locally to check for side effects of the merge
  • If defect, verify by running develop branch and reproducing defect, then running PR and reproducing fix
  • If feature, test running new feature, try creative ways to break it
  • CI status: all green or justified
  • Check that performance is not impacted (CI Linux results include performance check)
  • Run Unit Test(s) locally
  • Check any new function arguments for performance impacts
  • Verify IDF naming conventions and styles, memos and notes and defaults
  • If new idf included, locally check the err file and other outputs

…s are properly registered

Note: I made CoilCoolingDXFixture inherit SQLiteFixture instead of EnergyPlusFixture
@jmarrec jmarrec added the Defect Includes code to repair a defect in EnergyPlus label Sep 2, 2020
@@ -158,7 +160,7 @@ void CoilCoolingDXCurveFitOperatingMode::size(EnergyPlusData &state)
bool PrintFlag = true;

int SizingMethod = DataHVACGlobals::CoolingAirflowSizing;
std::string SizingString = "Rated Evaporator Air Flow Rate";
std::string SizingString = "Rated Evaporator Air Flow Rate [m3/s]";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add units

@@ -168,7 +170,7 @@ void CoilCoolingDXCurveFitOperatingMode::size(EnergyPlusData &state)
DataEnvironment::StdBaroPress, ratedInletAirTemp, ratedInletAirHumRat, RoutineName);

SizingMethod = DataHVACGlobals::CoolingCapacitySizing;
SizingString = "Rated Gross Total Cooling Capacity";
SizingString = "Rated Gross Total Cooling Capacity [W]";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add units

@@ -178,11 +180,23 @@ void CoilCoolingDXCurveFitOperatingMode::size(EnergyPlusData &state)
// Auto size condenser air flow to Total Capacity * 0.000114 m3/s/w (850 cfm/ton)
DataSizing::DataConstantUsedForSizing = this->ratedGrossTotalCap;
DataSizing::DataFractionUsedForSizing = 0.000114;
SizingString = "Rated Condenser Air Flow Rate";
SizingString = "Rated Condenser Air Flow Rate [m3/s]";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add units

Comment on lines +188 to +199

if (this->condenserType != AIRCOOLED) {
// Auto size Nominal Evaporative Condenser Pump Power to Total Capacity * 0.004266 w/w (15 W/ton)
SizingMethod = DataHVACGlobals::AutoCalculateSizing;
DataSizing::DataConstantUsedForSizing = this->ratedGrossTotalCap;
DataSizing::DataFractionUsedForSizing = 0.004266;
SizingString = "Nominal Evaporative Condenser Pump Power [W]";
TempSize = this->original_input_specs.nominal_evap_condenser_pump_power;
ReportSizingManager::RequestSizing(state, CompType, CompName, SizingMethod, SizingString, TempSize, PrintFlag, RoutineName);
this->nominalEvaporativePumpPower = TempSize;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Autosize per IO ref at 15 W/ton.

Copy link
Member

Choose a reason for hiding this comment

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

OK, so this is just using autocalculate, makes sense. Per @rraustad comment, once they are merged together, this code change will no longer compile. It will just require altering this code to use an AutocalculateSizer class instance instead.

Comment on lines +154 to +162
EXPECT_EQ(ratedEvapAirFlowRate, thisMode.ratedEvapAirFlowRate);
Real64 ratedGrossTotalCap = 18827.616766698276;
EXPECT_EQ(ratedGrossTotalCap, thisMode.ratedGrossTotalCap);
// Total Capacity * 0.000114 m3/s/w (850 cfm/ton)
Real64 ratedCondAirFlowRate = 0.000114 * ratedGrossTotalCap;
EXPECT_EQ(ratedCondAirFlowRate, thisMode.ratedCondAirFlowRate);
// Total Capacity * 0.004266 w/w (15 W/ton)
Real64 nominalEvaporativePumpPower = 0.004266 * ratedGrossTotalCap;
EXPECT_EQ(nominalEvaporativePumpPower, thisMode.nominalEvaporativePumpPower);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Test that autosizing worked.

Comment on lines +181 to +208
std::vector<TestQuery> testQueries({
TestQuery("Design Size Rated Evaporator Air Flow Rate", "m3/s", ratedEvapAirFlowRate),
TestQuery("Design Size Rated Gross Total Cooling Capacity", "W", ratedGrossTotalCap),
TestQuery("Design Size Rated Condenser Air Flow Rate", "m3/s", ratedCondAirFlowRate),
TestQuery("Design Size Nominal Evaporative Condenser Pump Power", "W", nominalEvaporativePumpPower),
});

for (auto &testQuery : testQueries) {

std::string query("SELECT Value From ComponentSizes"
" WHERE CompType = '" +
compType +
"'"
" AND CompName = '" +
compName +
"'"
" AND Description = '" +
testQuery.description + "'" + " AND Units = '" + testQuery.units + "'");

// execAndReturnFirstDouble returns -10000.0 if not found
Real64 return_val = SQLiteFixture::execAndReturnFirstDouble(query);

if (return_val < 0) {
EXPECT_TRUE(false) << "Query returned nothing for " << testQuery.displayString;
} else {
EXPECT_NEAR(testQuery.expectedValue, return_val, 0.01) << "Failed for " << testQuery.displayString;
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Test the SQL Output, ensure that we have the right units etc.

@rraustad
Copy link
Contributor

rraustad commented Sep 2, 2020

PR #8070 will need to incorporate these changes. In that branch, RequestSizing has been removed.

@Myoldmopar Myoldmopar added this to the EnergyPlus 9.4.0 milestone Sep 2, 2020
Copy link
Member

@Myoldmopar Myoldmopar left a comment

Choose a reason for hiding this comment

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

This looks great to me. I'm testing it locally.

Comment on lines +188 to +199

if (this->condenserType != AIRCOOLED) {
// Auto size Nominal Evaporative Condenser Pump Power to Total Capacity * 0.004266 w/w (15 W/ton)
SizingMethod = DataHVACGlobals::AutoCalculateSizing;
DataSizing::DataConstantUsedForSizing = this->ratedGrossTotalCap;
DataSizing::DataFractionUsedForSizing = 0.004266;
SizingString = "Nominal Evaporative Condenser Pump Power [W]";
TempSize = this->original_input_specs.nominal_evap_condenser_pump_power;
ReportSizingManager::RequestSizing(state, CompType, CompName, SizingMethod, SizingString, TempSize, PrintFlag, RoutineName);
this->nominalEvaporativePumpPower = TempSize;
}

Copy link
Member

Choose a reason for hiding this comment

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

OK, so this is just using autocalculate, makes sense. Per @rraustad comment, once they are merged together, this code change will no longer compile. It will just require altering this code to use an AutocalculateSizer class instance instead.

public:
protected:
void SetUp() override {
EnergyPlus::EnergyPlusFixture::SetUp(); // Sets up the base fixture first.
EnergyPlus::SQLiteFixture::SetUp(); // Sets up the base fixture first.
Copy link
Member

Choose a reason for hiding this comment

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

Gotcha, just made this inherit the sqlite fixture so that you can test the sql output.

@Myoldmopar
Copy link
Member

No build warnings on CI, and the diffs consist of the unit string. Nice. Building locally revealed a required code change; I'll push that up once I test a little more.

@Myoldmopar
Copy link
Member

OK, I tested this locally a bit. I am able to see the units showing up in the sizing reports no problem. I wasn't able to easily confirm the fix because our files that use this object are not evaporative condensers, and although the defect file is listed as added in the defect itself, I did not find a file uploaded or in dev support. Anyway, the unit test is great and I trust that this is working for you now. I am pushing my smol change up and am going to merge this in.

@Myoldmopar Myoldmopar merged commit de9073b into develop Sep 16, 2020
@Myoldmopar Myoldmopar deleted the 8066_CoilDXCurveOperatingMode_Sizing branch September 16, 2020 21:50
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Defect Includes code to repair a defect in EnergyPlus
Projects
None yet
8 participants