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

Make latent Degradation work for Variable Speed DX coil in Coil System #7928

Merged
merged 12 commits into from
Aug 1, 2020

Conversation

lgu1234
Copy link
Contributor

@lgu1234 lgu1234 commented Apr 14, 2020

Pull request overview

  1. Hard code HPTimeConstant at value of 45 seconds.
  2. Found a code bug to calculate QLatRated in CalcVarSpeedCoilCooling of the VariableSpeedCoils
  3. Unit test shows that latent degradation occurs
  4. Uploaded a test file

NOTE: ENHANCEMENTS MUST FOLLOW A SUBMISSION PROCESS INCLUDING A FEATURE PROPOSAL AND DESIGN DOCUMENT PRIOR TO SUBMITTING CODE

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

@lgu1234 lgu1234 added the Defect Includes code to repair a defect in EnergyPlus label Apr 14, 2020
@@ -5510,7 +5510,8 @@ namespace VariableSpeedCoils {
if (LatDegradModelSimFlag) {
// Calculate for SHReff using the Latent Degradation Model
if (NumIteration == 1) {
QLatRated = QLoadTotal - QSensible;
//match calculation in DXCoils at line 9899
QLatRated = VarSpeedCoil(DXCoilNum).MSRatedTotCap(SpeedCal) * (1.0 - VarSpeedCoil(DXCoilNum).MSRatedSHR(SpeedCal));
Copy link
Contributor

Choose a reason for hiding this comment

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

humm, I am not sure about this. I defer to @rraustad

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@EnergyArchmage Here is what I found. The original calculation makes QLatRated = 0.0, so that no latent degradation occurs. I just copied the code from DXCoils. It seems working. The unit test shows what happens.

Copy link
Contributor

Choose a reason for hiding this comment

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

These 2 equations should give the same answer. However, as @lgu1234 found, this model for some reason does set QLatRated = 0 (I just can't remember when or why that happened). The problem I see here is that if this equation is changed then where is QSensible corrected (or is it even needed on the fist pass)? This change only happens for NumIteration == 1 and should provide the correct QLatRated to use in the latent degradation model (but why didn't we see this in the Trane work?). I think I would rather see a correction to the total/sensible split during NumIteration == 1 instead of just correcting it after rated total and sensible capacity are calculated. That way this equation would not have to change and there would be confidence in the first pass of this model. I would also like to know if QLatRated is ever different during the simulation and if the need for 2 passes could be eliminated, but that's for a different issue. Maybe there are multiple QLatRated for each speed but I would think these would all be constants. This model will likely be deprecated (just don't know when) so maybe it's not worth the extra effort.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@EnergyArchmage @rraustad What I changed is based on the definition of QLatRated. Ideally, the NumIteration should be removed. Suggested code is listed below:

        if (LatDegradModelSimFlag) {
                QLatRated = VarSpeedCoil(DXCoilNum).MSRatedTotCap(SpeedCal) * (1.0 - VarSpeedCoil(DXCoilNum).MSRatedSHR(SpeedCal));
                QLatActual = QLoadTotal - QSensible;
                SHRss = QSensible / QLoadTotal;
                SHReff = CalcEffectiveSHR(
                    DXCoilNum, SHRss, CyclingScheme, RuntimeFrac, QLatRated, QLatActual, LoadSideInletDBTemp, LoadSideInletWBTemp);
                //       Update sensible capacity based on effective SHR
                QSensible = QLoadTotal * SHReff;
                goto LOOP_exit;
            }
        } else {

Any comments?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@EnergyArchmage @rraustad The removal of NumIteration is not correct. I retreat from my previous comments. I think to keep current fix and no more extra effort.

Copy link
Contributor

Choose a reason for hiding this comment

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

QLatRated should always be the same number, shouldn't it? It's the RATED latent capacity. This happens on the first loop in the calc routine. Then the load is calculated. Where QLoadTotal, Qsensible and Qlatent is found on the second loop. With your change you don't even need to do the first loop. Or is the first loop necessary for some other reason?

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 The change I made is to correct QLatRated calculation, so that the latent degradation is made to work. Otherwise, no SHR change at all. You may look at the unit test to find it out.
I am not sure if I need to do the first loop or not, because there are other calculations available in the first loop. I will post a new issue to deal with loop structure to place the QLatRated calculation in the right location, since issue you raised is beyond the scope of this issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm running the defect file now and see that the model is calculating QLoadTotal and SHR so that QLatRated = QLoadTotal - QSensible. So why then was this line of code needed? At the very least show a plot of before and after this line is changed.

Copy link
Contributor

Choose a reason for hiding this comment

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

I just checked the Trane code where the latent degradation model was corrected. This line of code (line 5514) was not changed and the latent model does work. So it looks like this change is not needed. Plots showing total/sensible/latent capacity with and without this change would help determine if it's really needed.

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 I revised the program by adding two output variables:

Variable Speed Cooling Coil SHR
Variable Speed Cooling Coil Effective SHR

The first variable is assigned before calling latent degradation, while the second variable is assigned after calling latent degradation. Attached is a zipped file containing simulation results

old.csv shows simulation results using
if (NumIteration == 1) {
QLatRated = QLoadTotal - QSensible;

new.csv shows simulation results using
if (NumIteration == 1) {
QLatRated = VarSpeedCoil(DXCoilNum).MSRatedTotCap(SpeedCal) * (1.0 - VarSpeedCoil(DXCoilNum).MSRatedSHR(SpeedCal));

The third file is the difference summary file. Surprisely, the differences are very small.

Then I debugged the program.

Using QLatRated = QLoadTotal - QSensible Calculated    
  Total SHR Sensible
  10089.88 0.807502 8147.598

Using QLatRated = VarSpeedCoil(DXCoilNum).MSRatedTotCap(SpeedCal) * (1.0 - VarSpeedCoil(DXCoilNum).MSRatedSHR(SpeedCal));

-- | -- | -- | -- | --
  | Total | SHR | Sensible | Latent
  | 9246.065 | 0.79 | 7304.392 | 1941.674

The difference is small using two different calculations. I don't have preference to use which one. The code is back to the original QLatRated calculation.

@lgu1234
Copy link
Contributor Author

lgu1234 commented Apr 15, 2020

The regression differences for DesiccantDehumidifierWithAirToAirCoil.idf are expected.

@nrel-bot
Copy link

@lgu1234 @lgentile it has been 28 days since this pull request was last updated.

@Myoldmopar
Copy link
Member

@lgu1234 @rraustad @EnergyArchmage I'll rely on you to resolve your conversation above and decide if this is ready or not. I just pulled develop in and pushed to get up to date results.

@Myoldmopar
Copy link
Member

CI came back happy again after running last week, just the one expected regression. If anyone can chime in to resolve the conversation that would be great. If not, do we need to set up a meeting to discuss?

@rraustad
Copy link
Contributor

rraustad commented Jul 2, 2020

In my mind, CalcTotCapSHR_VSWSHP should have returned the correct Total and SHR. I thought it would be better to fix the problem there and then there would be no need to change QLatRated = QLoadTotal - QSensible. Since that function returns Total and SHR, then SHR must be wrong. I do see in that function where SHRCalc is found first and then conditions are adjusted for dry coil, without recalculating SHRCalc. Maybe it's as simple as that.

@lgu1234
Copy link
Contributor Author

lgu1234 commented Jul 2, 2020

The purpose for this issue is to make latent degradation work. A possible problem in CalcTotCapSHR_VSWSHP needs to be investigated further. However, it is beyond the scope for this issue. My fix should be correct to calculate QLatRated. Even though CalcTotCapSHR_VSWSHP is fixed, the return value should be the same as my fix. The next issue should be to fix CalcTotCapSHR_VSWSHP.

@lgu1234
Copy link
Contributor Author

lgu1234 commented Jul 27, 2020

@rraustad The file is attached here,
TestResutls.zip.txt

@lgu1234
Copy link
Contributor Author

lgu1234 commented Jul 28, 2020

Differences are found in DesiccantDehumidifierWithAirToAirCoil, as expected, because latent degradation in CoilSystem:Cooling:DX is applied.

HVACDXSystem::ControlDXSystem(state, DXSystemNum, FirstHVACIteration, HXUnitOn);
Real64 SHR = VariableSpeedCoils::VarSpeedCoil(1).QSensible / VariableSpeedCoils::VarSpeedCoil(1).QLoadTotal;
EXPECT_NEAR(SHR, 0.811509, 0.0001);
DataLoopNode::Node(InletNode).MassFlowRate = 1.61576817;
Copy link
Contributor

Choose a reason for hiding this comment

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

The previous coil inputs resulted in an SHR = 0.811509 without latent degradation. Adding latent degradation would not change the SHR much, if at all, at this high PLR. Then the mass flow rate is changed and latent degradation inputs added. The resulting SHR = 1, but what is that compared to? It's at a different mass flow rate. What was the SHR before the latent degradation inputs were added using the new mass flow rate? Or just adjust the coil inputs or set point to give a lower SHR before adding the latent degradation inputs. The point here is that this unit test doesn't really test the latent degradation model.

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 I agree with your opinion that the unit test is not very meaningful. The unit test is based on the original calculation of QLatRated
QLatRated = QLoadTotal - QSensible;

If QLatRated is calculated based on my revision from its definition:

                QLatRated = VarSpeedCoil(DXCoilNum).MSRatedTotCap(SpeedCal) * (1.0 - VarSpeedCoil(DXCoilNum).MSRatedSHR(SpeedCal));

the meaningful unit test is obtained from my previous check-in:

//DataLoopNode::Node(InletNode).MassFlowRate = 1.61576817;
VariableSpeedCoils::VarSpeedCoil(1).Twet_Rated = 1000.0;
VariableSpeedCoils::VarSpeedCoil(1).Gamma_Rated = 1.5;
HVACDXSystem::ControlDXSystem(state, DXSystemNum, FirstHVACIteration, HXUnitOn);
SHR = VariableSpeedCoils::VarSpeedCoil(1).QSensible / VariableSpeedCoils::VarSpeedCoil(1).QLoadTotal;
//EXPECT_NEAR(SHR, 1.0, 0.0001);
EXPECT_NEAR(SHR, 0.878975, 0.0001);

When I ran a test file using two different QLatRated calculations, the differences are minor, as shown in the previously attached file.

As mentioned before, if conditions are OK, the original calculation should get the same results as one from its definition. However, the same results would be very rare in simulations. The calculation results of my revision are based on inputs. The conditions needed to get same results from both calculations are:

  1. Indoor wet bulb temperature and outdoor dry bulb temperature are at rated conditions
  2. All multipliers at rated conditions are equal to 1.0

Otherwise, different results are obtained.

In order to get meaningful unit test, my revision should be used. That is a reason I made the change,

@rraustad
Copy link
Contributor

I looked at the SHR of the coil with and without the change to the model. They both look the same.

image

In fact they are identical.

image

So there must be something wrong with the unit test. I noticed that the air flow rate used was the speed 5 air flow rate. This is a very high flow rate for a VS coil that is operating at speed 1. I changed the node flow rate to match speed 1 and the SHRs made sense. I also added another unit test at more reasonable zone RH.

@rraustad
Copy link
Contributor

@Myoldmopar this issue is now resolved and I approve this PR.

Copy link
Contributor

@rraustad rraustad left a comment

Choose a reason for hiding this comment

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

The changes made allow the VS coil model to use latent degradation.

@rraustad
Copy link
Contributor

Just need to wait for CI to run and see if @lgu1234 wants to change things back to the way he had them.

@rraustad
Copy link
Contributor

The only diff is DesiccantDehumidifierWithAirToAirCoil which does have a VS coil and latent degradation is turned on. I would expect diff's.

Coil:Cooling:DX:VariableSpeed,
  Desiccant DXSystem VS Cooling Coil,  !- Name
  1000,                    !- Nominal Time for Condensate to Begin Leaving the Coil {s}
  0.4,                     !- Initial Moisture Evaporation Rate Divided by Steady-State AC Latent Capacity {dimensionless}

@rraustad
Copy link
Contributor

@lgu1234 if I don't hear from you I will merge this once CI completes.

@rraustad
Copy link
Contributor

rraustad commented Aug 1, 2020

Hmm, 8 hours and CI has not completed. @lgu1234 do you have any comments on these changes?

@lgu1234
Copy link
Contributor Author

lgu1234 commented Aug 1, 2020

@rraustad No more comments. Thanks.

@rraustad rraustad merged commit 5bbc52d into develop Aug 1, 2020
@Myoldmopar Myoldmopar deleted the 172262406-Issue7671 branch February 20, 2021 14:18
# 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
Development

Successfully merging this pull request may close these issues.

9 participants