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

Various DX coil fixes #8576

Merged
merged 11 commits into from
Mar 22, 2021
Merged

Various DX coil fixes #8576

merged 11 commits into from
Mar 22, 2021

Conversation

mjwitte
Copy link
Contributor

@mjwitte mjwitte commented Mar 2, 2021

Pull request overview

  • This contains various fixes related to the old DX coils taken from the Coil:DX:Cooling work.
  • Use PLR, not RuntimeFraction to calculate leaving conditions for 2-stage operation (Coil:Cooling:DX:TwoStageWithHumidityControlMode).
  • Fix Multispeed DX massflow weighting between speeds (Coil:Cooling:DX:Multispeed)
  • Fix Multispeed DX outlet conditions at low speed (Coil:Cooling:DX:Multispeed)
  • Allow 2-speed DX to operate as variable speed (Coil:Cooling:DX:TwoSpeed)

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

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 changed the title Various (old) DX coil fixes Various DX coil fixes Mar 2, 2021
@mjwitte mjwitte added the Defect Includes code to repair a defect in EnergyPlus label Mar 2, 2021
@mjwitte mjwitte added this to the EnergyPlus 9.5.0 milestone Mar 2, 2021
@mjwitte mjwitte mentioned this pull request Mar 3, 2021
25 tasks
@mjwitte mjwitte marked this pull request as ready for review March 5, 2021 21:08
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. Note that UnitarySystem changes overlap (duplicate?) changes in #8558.

Comment on lines -617 to +618
(1.0 - S12RuntimeFraction) * S1OutletAirEnthalpy + S12RuntimeFraction * S12OutletAirEnthalpy;
state.dataDXCoils->DXCoil(DXCoilNum).OutletAirHumRat = (1.0 - S12RuntimeFraction) * S1OutletAirHumRat + S12RuntimeFraction * S12OutletAirHumRat;
(1.0 - S2PLR) * S1OutletAirEnthalpy + S2PLR * S12OutletAirEnthalpy;
state.dataDXCoils->DXCoil(DXCoilNum).OutletAirHumRat = (1.0 - S2PLR) * S1OutletAirHumRat + S2PLR * S12OutletAirHumRat;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Use PLR, not RuntimeFraction to calculate leaving conditions for 2-stage operation (Coil:Cooling:DX:TwoStageWithHumidityControlMode). RuntimeFraction is appropriate for power consumption.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed.

Comment on lines 12196 to 12197
if (FanOpMode == ContFanCycCoil) {
Real64 MinAirHumRat(0.0); // set to zero because MinAirHumRat is unused argument
Hfg = PsyHfgAirFnWTdb(MinAirHumRat, HSOutletAirDryBulbTemp * SpeedRatio + (1.0 - SpeedRatio) * LSOutletAirDryBulbTemp);
// Average outlet HR
OutletAirHumRat = InletAirHumRat - state.dataDXCoils->DXCoil(DXCoilNum).LatCoolingEnergyRate / Hfg / state.dataDXCoils->DXCoil(DXCoilNum).InletAirMassFlowRate;
} else {
OutletAirHumRat = (HSOutletAirHumRat * SpeedRatio) + (LSOutletAirHumRat * (1.0 - SpeedRatio));
}
OutletAirHumRat =
((HSOutletAirHumRat * SpeedRatio * MSHPMassFlowRateHigh) + (LSOutletAirHumRat * (1.0 - SpeedRatio) * MSHPMassFlowRateLow)) /
state.dataDXCoils->DXCoil(DXCoilNum).InletAirMassFlowRate;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Multispeed DX fix massflow weighting between speeds (2398ce6)

Copy link
Contributor

Choose a reason for hiding this comment

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

I recall this from @lgu1234 a while back.

Comment on lines 12223 to 12224
// Now reset runtime fraction to 1.0 (because LS is running the full timestep)
state.dataDXCoils->DXCoil(DXCoilNum).CoolingCoilRuntimeFraction = 1.0;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This inside a big if (SpeedNum > 1 && SingleMode == 0) { so this means it running at speed 2 or higher. (bd8aa38)

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense.

OutletAirHumRat = LSOutletAirHumRat;
OutletAirDryBulbTemp = LSOutletAirDryBulbTemp;
if (FanOpMode == ContFanCycCoil) {
// outlet conditions are average of inlet and low speed weighted by CycRatio
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Multispeed DX fix outlet conditions at low speed (bd8aa38)

Copy link
Contributor

Choose a reason for hiding this comment

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

I see in here where OutletAir* variables are calculated. Oh, there it is, in the else they are also set for cycling fan. OK then.

Comment on lines 2143 to 2144
} else if (this->m_CoolingCoilType_Num == DataHVACGlobals::CoilDX_MultiSpeedCooling) {
} else if (this->m_CoolingCoilType_Num == DataHVACGlobals::CoilDX_MultiSpeedCooling ||
this->m_CoolingCoilType_Num == DataHVACGlobals::CoilDX_CoolingTwoSpeed) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Allow 2-speed dx to have variable speeds (ea02126)

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 actually missing from develop? And now things line up better?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rraustad You actually made this commit a year ago. ea02126 Ring a bell?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, but at least I'm consistent. And that whole conversation of what does idle speed anything mean is still a little confusing. I see now there is a call to DXCoils::SimDXCoilMultiSpeed. That is the correct call for 2-spd coil? I've lost track but thought is was a different function.

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, that's fine. There are further checks on coil type in that function.
line 343 if (SELECT_CASE_var == CoilDX_CoolingTwoSpeed) {

@@ -2179,6 +2180,15 @@ namespace UnitarySystems {
this->m_IdleVolumeAirRate = this->m_MaxNoCoolHeatAirVolFlow;
this->m_IdleMassFlowRate = this->MaxNoCoolHeatAirMassFlow;
this->m_IdleSpeedRatio = this->m_IdleVolumeAirRate / this->m_DesignFanVolFlowRate;
} else {
for ( Iter = this->m_NumOfSpeedCooling; Iter > 0; --Iter ) {
this->m_CoolVolumeFlowRate[ Iter ] = this->m_MaxCoolAirVolFlow * Iter / this->m_NumOfSpeedCooling;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

More allow 2-speed dx to be variable speed (ea02126)

Copy link
Contributor

Choose a reason for hiding this comment

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

and this goes with the 2-speed coil addition to this section.

@@ -4414,7 +4424,13 @@ namespace UnitarySystems {

} else { // mine data from DX cooling coil

if (thisSys.m_CoolingCoilType_Num == DataHVACGlobals::CoilDX_CoolingTwoSpeed) thisSys.m_NumOfSpeedCooling = 2;
if (thisSys.m_CoolingCoilType_Num == DataHVACGlobals::CoilDX_CoolingTwoSpeed) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

More allow 2-speed dx to be variable speed (ea02126)

state.dataDXCoils->DXCoil(DXCoilNum).TotalCoolingEnergyRate);
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

The above deleted code looks like it's catching up to develop.

@@ -7286,6 +7290,7 @@ namespace UnitarySystems {
}

if (state.dataUnitarySystems->unitarySys[sysNum].m_CoolingCoilType_Num == DataHVACGlobals::CoilDX_MultiSpeedCooling ||
state.dataUnitarySystems->unitarySys[sysNum].m_CoolingCoilType_Num == DataHVACGlobals::CoilDX_CoolingTwoSpeed ||
state.dataUnitarySystems->unitarySys[sysNum].m_CoolingCoilType_Num == DataHVACGlobals::CoilDX_Cooling ||
state.dataUnitarySystems->unitarySys[sysNum].m_HeatingCoilType_Num == DataHVACGlobals::CoilDX_MultiSpeedHeating ||
state.dataUnitarySystems->unitarySys[sysNum].m_HeatingCoilType_Num == DataHVACGlobals::Coil_HeatingElectric_MultiStage ||
Copy link
Contributor

Choose a reason for hiding this comment

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

I see, you moved the report variable set up to here (but didn't get rid of the "if" in above code at 7207).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch - will clean that up.

@@ -9077,7 +9082,10 @@ namespace UnitarySystems {
// RETURN if the moisture load is met
if (state.dataUnitarySystems->MoistureLoad >= 0.0 || state.dataUnitarySystems->MoistureLoad >= TempLatOutput) return;
// Multimode does not meet the latent load, only the sensible load with or without HX active
// what if there is a heating load for a system using Multimode?
Copy link
Contributor

Choose a reason for hiding this comment

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

May need to create a new issue to see if this works as expected.

Real64 totalAncillaryPower =
thisSys->m_AncillaryOnPower * thisSys->m_PartLoadFrac + thisSys->m_AncillaryOffPower * (1.0 - thisSys->m_PartLoadFrac);
EXPECT_NEAR(totalAncillaryPower, thisSys->m_TotalAuxElecPower, 0.00000001);
// at PLR very near 0.5, m_TotalAuxElecPower should be very near 75 W.
EXPECT_NEAR(74.694, thisSys->m_TotalAuxElecPower, 0.0001);
EXPECT_NEAR(74.4036, thisSys->m_TotalAuxElecPower, 0.0001);
Copy link
Contributor

Choose a reason for hiding this comment

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

small diff's in unit tests, good to go so far.

@mjwitte
Copy link
Contributor Author

mjwitte commented Mar 17, 2021

@rraustad Thanks for the review.

@Myoldmopar
Copy link
Member

@mjwitte my global branch is about to merge. I don't anticipate major conflicts with this branch. I believe @mitchute also has another globals branch soon to be ready to merge. After that I'm back to trying to get the other PRs reviewed and merged for a little while, then more globals tomorrow afternoon. All that to say, if any more conflicts arise, I'm happy to resolve them. 👍

@mjwitte
Copy link
Contributor Author

mjwitte commented Mar 17, 2021

@Myoldmopar Much appreciated. I'm done with this for the time being. I have not looked at the diffs to make sure there's nothing ridiculous in here.

@Myoldmopar
Copy link
Member

Pulled develop in, there was only a tiny conflict even after all those PRs. I ran the unit test suite, and I am starting to see some SQL related unit tests fail, but they only seem to be happening when the unit tests are run serially, and it's not consistent. I don't have time to investigate right now, so I'm just going to pretend for tonight like they aren't happening.

I'll run the regressions locally, and assuming CI is OK with my merge I'll take a peek into the diffs and we'll see if we can drop this right in tomorrow morning, then follow it up with the other coil branch.

@Myoldmopar
Copy link
Member

I am able to reproduce the diffs locally as they are on CI.

  • EIO diffs: Warmup convergence, tiny
  • ERR diffs: Reduced hvac iteration warnings
  • Table diffs: Small changes in peak electricity usage, generally < 1%
  • ESO diffs: Values generally changed on the order of e-8, unitary system electricity off by ~1%
  • MTR diffs: HVAC electricity off by less than 1%

I took the big diff math files and re-ran them with only annual results and got zero out of range math diffs. I believe this is good to go in. Thanks @mjwitte and @rraustad ! This is great!

@Myoldmopar Myoldmopar merged commit f1a13ce into develop Mar 22, 2021
@Myoldmopar Myoldmopar deleted the CoilBaselineOldCoilFixes branch March 22, 2021 13:22
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
Development

Successfully merging this pull request may close these issues.

8 participants