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

Enhancing Multi-speed Coil Speed Level Control Actuators #9185

Merged
merged 12 commits into from
Dec 3, 2021

Conversation

xuanluo113
Copy link
Contributor

@xuanluo113 xuanluo113 commented Nov 12, 2021

Pull request overview

Previously in the 9.6.0 release, we added the EMS actuators to control the speed level of the cooling and heating multispeed coils, namely Coil:Cooling:DX:MultiSpeed and Coil:Heating:DX:Multispeed. As an enhancement, we propose to extend the EMS functionality to a more general coil type in EnergyPlus, Coil:Cooling:DX, which will replace the single speed, two speed, variable speed, multi-speed, and multi-stage coils going forward.

Result of the testfiles/EMSLoadBasedMultiSpeedDXCoilOverrideControl.idf

Screen Shot 2021-11-23 at 10 00 21 AM

Result of the testfiles/EMSSetpointBasedMultiSpeedDXCoilOverrideControl.idf

Screen Shot 2021-11-23 at 10 00 37 AM

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

@xuanluo113 xuanluo113 added the NewFeature Includes code to add a new feature to EnergyPlus label Nov 12, 2021
@xuanluo113 xuanluo113 added this to the EnergyPlus 2022.1 milestone Nov 12, 2021
@xuanluo113 xuanluo113 self-assigned this Nov 12, 2021
@mjwitte
Copy link
Contributor

mjwitte commented Nov 18, 2021

@xuanluo113 This is straightforward enough. If I'm following correctly, since the actuator is applied to the unitary system, there is no change in inputs for this, just doc and code changes to extend this to work with Coil:Cooling:DX. When updating the documentation, some reformatting would be helpful (the Unitary Equipment actuator section is a bit hard to navigate).

@xuanluo113
Copy link
Contributor Author

The branch is ready for review with a new unit test and three test files. Results are illustrated in the overview.

@xuanluo113
Copy link
Contributor Author

@mjwitte @Myoldmopar Mike and Edwin, this branch is ready for review, and I wonder if you could take a look. As for the documentation, I'm not sure what is a better place to add the EMS description other than the Unitary Equipment, since the actuators are based on unitary systems.

Copy link
Contributor

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

Regarding the documentation, it's fine to include it with the unitarysystem actuators, but that section could use some subheadings or something to make it more readable.

this->m_SpeedNum = this->m_NumOfSpeedCooling;
CycRatio = this->m_CoolingCycRatio = 1.0;
PartLoadFrac = SpeedRatio = this->m_CoolingSpeedRatio = 1.0;
this->m_CoilSpeedErrIdx++;
Copy link
Contributor

Choose a reason for hiding this comment

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

This index should not be incremented here. This is an index to this particular recurring error message. If it's zero ShowRecurringWarningErrorAtEnd will set it to the next available index. There should also be a regular warning message issued the first time this occurs (i.e. when m_CoilSpeedErrIdx=0) . See UnitarySystem.cc lines 9347ff for an example of how to use ShowRecurringWarningErrorAtEnd.

Comment on lines 11959 to 11962
} else if (this->m_CoolingSpeedNum == 1) {
SpeedRatio = this->m_CoolingSpeedRatio = 0.0;
CycRatio = this->m_CoolingCycRatio = this->m_EMSOverrideCoilSpeedNumValue - floor(this->m_EMSOverrideCoilSpeedNumValue);
if (useMaxedSpeed || this->m_CoolingCycRatio == 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure this should be an else if. Looks like useMaxedSpeed will always be false here.

Comment on lines 11966 to 11968
} else {
CycRatio = this->m_CoolingCycRatio = 1.0;
SpeedRatio = this->m_CoolingSpeedRatio = this->m_EMSOverrideCoilSpeedNumValue - floor(this->m_EMSOverrideCoilSpeedNumValue);
Copy link
Contributor

Choose a reason for hiding this comment

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

What if m_EMSOverrideCoilSpeedNumValue is <= 0?

@xuanluo113
Copy link
Contributor Author

@mjwitte Hi Mike. I've fixed the recurring warning issues, and added some subsubsection headings in the doc.

Copy link
Contributor

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

This is better, but there is a failed unit test now.

Thanks for adding the subheadings in the unitary equipment EMS actuators section. Please add one more "Unitary System Sizing" before the paragraph starting with "Actuators are available for overriding the autosize values".

ShowContinueError(state,
" Exceeding maximum coil speed level. Speed level is set to the maximum coil speed level allowed.");
}
ShowRecurringWarningErrorAtEnd(
Copy link
Contributor

Choose a reason for hiding this comment

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

This is better. I was going to comment that the two error messages here will conflict once the m_CoilSpeedErrIdx is set. But now I see that ShowRecurringWarningErrorAtEnd ignores the incoming index and searches for a matching error string. A test file which sets the coil speed to values higher than max peed, and to negative values, does produce both recurring error messages. So, this is fine as-is.

@@ -12281,6 +12292,35 @@ namespace UnitarySystems {
} else if (this->m_DehumidificationMode == 1) {
OperationMode = DataHVACGlobals::coilEnhancedMode;
}

// if (this->m_EMSOverrideCoilSpeedNumOn) {
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a block of commented code here that needs to be removed.

@@ -249,10 +249,12 @@ add_simulation_test(IDF_FILE EMSCustomOutputVariable.idf EPW_FILE USA_IL_Chicago
add_simulation_test(IDF_FILE EMSCustomSchedule.idf EPW_FILE USA_IL_Chicago-OHare.Intl.AP.725300_TMY3.epw)
add_simulation_test(IDF_FILE EMSDemandManager_LargeOffice.idf EPW_FILE USA_IL_Chicago-OHare.Intl.AP.725300_TMY3.epw)
add_simulation_test(IDF_FILE EMSDiscreteAirSystemSizes.idf EPW_FILE USA_IL_Chicago-OHare.Intl.AP.725300_TMY3.epw)
add_simulation_test(IDF_FILE EMSLoadBasedMultiSpeedDXCoilOverrideControl.idf EPW_FILE USA_IL_Chicago-OHare.Intl.AP.725300_TMY3.epw)
Copy link
Contributor

Choose a reason for hiding this comment

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

This new test file has some warnings:

   *************  ** Warning ** SimHVAC: Exceeding Maximum iterations for all HVAC loops, during JAN14 continues
   *************  **   ~~~   **   This error occurred 44 total times;
   *************  **   ~~~   **   during Warmup 0 times;
   *************  **   ~~~   **   during Sizing 0 times.
   *************
   *************  ** Warning ** Wrong coil speed EMS override value, for unit="SYS 1 FURNACE DX COOL HEATING COIL". Exceeding maximum coil speed level. Speed level is set to the maximum coil speed level allowed.
   *************  **   ~~~   **   This error occurred 63 total times;
   *************  **   ~~~   **   during Warmup 54 times;
   *************  **   ~~~   **   during Sizing 0 times.
   *************  **   ~~~   **   Max=1.  Min=1.

Copy link
Contributor

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

@xuanluo113
Docs are good.
Unit tests pass.
Output variable warnings are gone (I just pushed up that change for the other idf).
Still troubled by these warnings in EMSLoadBasedMultiSpeedDXCoilOverrideControl:

   *************  ** Warning ** SimHVAC: Exceeding Maximum iterations for all HVAC loops, during JAN14 continues
   *************  **   ~~~   **   This error occurred 44 total times;
   *************  **   ~~~   **   during Warmup 0 times;
   *************  **   ~~~   **   during Sizing 0 times.
   *************
   *************  ** Warning ** Wrong coil speed EMS override value, for unit="SYS 1 FURNACE DX COOL HEATING COIL". Exceeding maximum coil speed level. Speed level is set to the maximum coil speed level allowed.
   *************  **   ~~~   **   This error occurred 63 total times;
   *************  **   ~~~   **   during Warmup 54 times;
   *************  **   ~~~   **   during Sizing 0 times.
   *************  **   ~~~   **   Max=1.  Min=1.
   *************
   *************  ** Warning ** SimHVAC: Exceeding Maximum iterations for all HVAC loops, during JULY7 continues
   *************  **   ~~~   **   This error occurred 4 total times;
   *************  **   ~~~   **   during Warmup 0 times;
   *************  **   ~~~   **   during Sizing 0 times.

If there's not an obvious fix for these, please post a followup issue to investigate further.

@mjwitte
Copy link
Contributor

mjwitte commented Dec 3, 2021

Merging this. Will post a followup issue about the warnings in EMSLoadBasedMultiSpeedDXCoilOverrideControl.

@mjwitte mjwitte merged commit e6e681d into develop Dec 3, 2021
@mjwitte mjwitte deleted the coil-speed-actuator branch December 3, 2021 21:02
yujiex pushed a commit that referenced this pull request Jan 27, 2022
Enhancing Multi-speed Coil Speed Level Control Actuators
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NewFeature Includes code to add a new feature to EnergyPlus
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants