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.

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

Fix Controller:MechanicalVentilation ProportionalControlBasedOnDesignOccupancy #10268

Merged
merged 8 commits into from
Nov 2, 2023

Conversation

mjwitte
Copy link
Contributor

@mjwitte mjwitte commented Oct 10, 2023

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

@mjwitte mjwitte added the Defect Includes code to repair a defect in EnergyPlus label Oct 10, 2023
@mjwitte
Copy link
Contributor Author

mjwitte commented Oct 19, 2023

Single-zone unit test passes in develop, but 3-zone unit test fails in develop.

[----------] 2 tests from EnergyPlusFixture
[ RUN      ] EnergyPlusFixture.CO2ControlDesignOccupancyTest
[       OK ] EnergyPlusFixture.CO2ControlDesignOccupancyTest (14693 ms)
[ RUN      ] EnergyPlusFixture.CO2ControlDesignOccupancyTest3Zone
C:\MJWGit\EnergyPlus\tst\EnergyPlus\unit\MixedAir.unit.cc(1263): error: The difference between 3 * expectedOAMassFlow and oaController.OAMassFlow is 0.0039677399219509683, which exceeds 0.00001, where
3 * expectedOAMassFlow evaluates to 0.024854760000000004,
oaController.OAMassFlow evaluates to 0.020887020078049035, and
0.00001 evaluates to 1.0000000000000001e-05.
C:\MJWGit\EnergyPlus\tst\EnergyPlus\unit\MixedAir.unit.cc(1264): error: The difference between 3 * expectedOAMassFlow / oaController.MixMassFlow and oaController.MinOAFracLimit is 0.0019449705499759648, which exceeds 0.00001, where
3 * expectedOAMassFlow / oaController.MixMassFlow evaluates to 0.012183705882352942,
oaController.MinOAFracLimit evaluates to 0.010238735332376977, and
0.00001 evaluates to 1.0000000000000001e-05.
[  FAILED  ] EnergyPlusFixture.CO2ControlDesignOccupancyTest3Zone (37483 ms)
[----------] 2 tests from EnergyPlusFixture (52178 ms total)

@mjwitte mjwitte marked this pull request as ready for review October 19, 2023 18:52
Copy link
Contributor Author

@mjwitte mjwitte left a comment

Choose a reason for hiding this comment

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

Code walkthru.

for (int peopleNum = 1; peopleNum <= state.dataHeatBal->TotPeople; ++peopleNum) {
for (auto &thisMechVentZone : vent_mech.VentMechZone) {
if (state.dataHeatBal->People(peopleNum).ZonePtr == thisMechVentZone.zoneNum) {
thisMechVentZone.peopleIndexes.push_back(peopleNum);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Save list of applicable people instances.

@@ -3672,19 +3684,18 @@ Real64 VentilationMechanicalProps::CalcMechVentController(EnergyPlusData &state,
// new local variables for DCV
// Zone OA flow rate based on each calculation method [m3/s]
std::array<Real64, static_cast<int>(DataSizing::OAFlowCalcMethod::Num)> ZoneOACalc{0.0};
Real64 CO2PeopleGeneration = 0; // CO2 generation from people at design level
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Narrow scope for this.

Copy link
Member

Choose a reason for hiding this comment

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

That's a great thing.

Comment on lines -3949 to +3961
for (int PeopleNum = 1; PeopleNum <= state.dataHeatBal->TotPeople; ++PeopleNum) {
if (state.dataHeatBal->People(PeopleNum).ZonePtr != ZoneNum) continue;
Real64 CO2PeopleGeneration = 0.0;
for (int const PeopleNum : thisMechVentZone.peopleIndexes) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Loop through only the applicable people instances, not all of them. Also reset sum to zero for each zone (previously this kept accumulating across zones).

@@ -355,6 +355,7 @@ add_simulation_test(IDF_FILE HeatPumpIAQP_DCV.idf EPW_FILE USA_IL_Chicago-OHare.
add_simulation_test(IDF_FILE HeatPumpIAQP_GenericContamControl.idf EPW_FILE USA_IL_Chicago-OHare.Intl.AP.725300_TMY3.epw)
add_simulation_test(IDF_FILE HeatPumpProportionalControl_DCV.idf EPW_FILE USA_IL_Chicago-OHare.Intl.AP.725300_TMY3.epw)
add_simulation_test(IDF_FILE HeatPumpProportionalControl_DCVDesignRate.idf EPW_FILE USA_IL_Chicago-OHare.Intl.AP.725300_TMY3.epw)
add_simulation_test(IDF_FILE HeatPumpProportionalControl_DCVDesignOccAllZones.idf EPW_FILE USA_IL_Chicago-OHare.Intl.AP.725300_TMY3.epw)
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 a test file that uses this control type.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good to me.

Real64 zoneCO2Max = 431.08678;
Real64 zoneCO2Min = 400.0;

// Case 1 - Zone CO2 greater than CO2 Max, so OA flow is flow/area+flow/person
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Single-zone test, three cases.

EXPECT_NEAR(expectedOAMassFlow / oaController.MixMassFlow, oaController.MinOAFracLimit, 0.00001);
}

TEST_F(EnergyPlusFixture, CO2ControlDesignOccupancyTest3Zone)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

3-zone test, identical to single-zone test but with 3 zones, expect 3x OA flow rates.

@Myoldmopar
Copy link
Member

The Windows build failure is unrelated, as is the one small diff that pops up occasionally. I'll pull develop in and build while looking this over.

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.

Looks like fine little fix and cleanup with sufficient tests to me.

@@ -3672,19 +3684,18 @@ Real64 VentilationMechanicalProps::CalcMechVentController(EnergyPlusData &state,
// new local variables for DCV
// Zone OA flow rate based on each calculation method [m3/s]
std::array<Real64, static_cast<int>(DataSizing::OAFlowCalcMethod::Num)> ZoneOACalc{0.0};
Real64 CO2PeopleGeneration = 0; // CO2 generation from people at design level
Copy link
Member

Choose a reason for hiding this comment

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

That's a great thing.

@@ -286,6 +286,7 @@ namespace MixedAir {
DataSizing::OAFlowCalcMethod ZoneOAFlowMethod = DataSizing::OAFlowCalcMethod::PerPerson; // OA flow method for each zone
int ZoneOASchPtr = 0; // Index to the outdoor air schedule for each zone (from DesignSpecification:OutdoorAir or default)
Real64 OAPropCtlMinRateSchPtr = 0; // Outdoor design OA flow rate schedule from DesignSpecification:OutdoorAir
EPVector<int> peopleIndexes; // List of People objects in this zone (for SystemOAMethod == DataSizing::SysOAMethod::ProportionalControlDesOcc)
Copy link
Member

Choose a reason for hiding this comment

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

Yay, a nice little EPVector!

@@ -355,6 +355,7 @@ add_simulation_test(IDF_FILE HeatPumpIAQP_DCV.idf EPW_FILE USA_IL_Chicago-OHare.
add_simulation_test(IDF_FILE HeatPumpIAQP_GenericContamControl.idf EPW_FILE USA_IL_Chicago-OHare.Intl.AP.725300_TMY3.epw)
add_simulation_test(IDF_FILE HeatPumpProportionalControl_DCV.idf EPW_FILE USA_IL_Chicago-OHare.Intl.AP.725300_TMY3.epw)
add_simulation_test(IDF_FILE HeatPumpProportionalControl_DCVDesignRate.idf EPW_FILE USA_IL_Chicago-OHare.Intl.AP.725300_TMY3.epw)
add_simulation_test(IDF_FILE HeatPumpProportionalControl_DCVDesignOccAllZones.idf EPW_FILE USA_IL_Chicago-OHare.Intl.AP.725300_TMY3.epw)
Copy link
Member

Choose a reason for hiding this comment

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

Sounds good to me.

@Myoldmopar
Copy link
Member

Everything tests out perfectly fine, and the changes look good. Merging this. Thanks @mjwitte

@Myoldmopar Myoldmopar merged commit b99411d into develop Nov 2, 2023
10 checks passed
@Myoldmopar Myoldmopar deleted the VentMechCO2Control9969 branch November 2, 2023 19:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Defect Includes code to repair a defect in EnergyPlus
Projects
None yet
7 participants