Make Water-to-Air Heat Pump cycling and latent degradation consistent with other coils

[nealkruis]

Pull request overview

• Fixes Water-to-Air Heat Pump Cycling Model Issues #9966
• Moves latent cycling and latent degradation inputs from parent objects into the coil objects (for consistency with other coil objects)
• Replaces cycling model from DOE-2 air source equipment that was inappropriately applied to water source equipment with a generic Part Load Factor curve
• Addresses issue where cycling losses included off-cycle power that was being added to the source-side water stream, and could cause unrealistically high outlet temperatures at low part load

TODO:

• Update documentation
• Create transition rules?
• Refactor variable names that don't reflect they are now only used for latent degradation modeling

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

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

[nealkruis]

I will make documentation changes once a reviewer has approved the IDD changes (just let me know).

For transition rules:

• HVACTemplate:Zone:WaterToAirHeatPump:
  • "Remove N20, \field Fraction of On-Cycle Power Use"
• ZoneHVAC:WaterToAirHeatPump:
  • Move "N7, \field Maximum Cycling Rate" to child object
  • Move "N8, \field Heat Pump Time Constant" to child object
  • Remove "N9, \field Fraction of On-Cycle Power Use"
  • Move "N10, \field Heat Pump Fan Delay Time" to child object
• AirLoopHVAC:UnitarySystem:
  • Move "N19, \field Maximum Cycling Rate" to child object
  • Move "N20, \field Heat Pump Time Constant" to child object
  • Remove "N21, \field Fraction of On-Cycle Power Use"
  • Move "N22, \field Heat Pump Fan Delay Time" to child object
• AirLoopHVAC:UnitaryHeatPump:WaterToAir:
  • Move "N4, \field Maximum Cycling Rate" to child object
  • Move "N5, \field Heat Pump Time Constant" to child object
  • Remove "N6, \field Fraction of On-Cycle Power Use"
  • Move "N7, \field Heat Pump Fan Delay Time" to child object
• Coil:Cooling:DX:VariableSpeed:
  • Insert "N7, \field Maximum Cycling Rate" from parent or leave blank
  • Insert "N8, \field Latent Capacity Time Constant" from parent's "Heat Pump Time Constant" or leave blank
  • Insert "N9, \field Fan Delay Time" from parent or leave blank
• Coil:Cooling:WaterToAirHeatPump:ParameterEstimation:
  • Insert "A8, \field Part Load Fraction Correlation Curve Name" and add Curve:Linear using correlation below.
  • Insert "N21, \field Maximum Cycling Rate" from parent or leave blank
  • Insert "N22, \field Latent Capacity Time Constant" from parent's "Heat Pump Time Constant" or leave blank
  • Insert "N23, \field Fan Delay Time" from parent or leave blank
• Coil:Heating:WaterToAirHeatPump:ParameterEstimation:
  • Insert "A8, \field Part Load Fraction Correlation Curve Name" and add Curve:Linear using correlation below.
• Coil:Cooling:WaterToAirHeatPump:EquationFit:
  • Insert "A9, \field Part Load Fraction Correlation Curve Name" and add Curve:Linear using correlation below.
  • Insert "N13, \field Maximum Cycling Rate" from parent or leave blank
  • Insert "N14, \field Latent Capacity Time Constant" from parent's "Heat Pump Time Constant" or leave blank
  • Insert "N15, \field Fan Delay Time" from parent or leave blank
• Coil:Heating:WaterToAirHeatPump:EquationFit:
  • Insert "A8, \field Part Load Fraction Correlation Curve Name" and add Curve:Linear using correlation below.
• Coil:Cooling:WaterToAirHeatPump:VariableSpeedEquationFit:
  • Insert "N8, \field Maximum Cycling Rate" from parent or leave blank
  • Insert "N9, \field Latent Capacity Time Constant" from parent's "Heat Pump Time Constant" or leave blank
  • Insert "N10, \field Fan Delay Time" from parent or leave blank

Part Load Fraction correlation: (1-Cd) + Cd*PLR

Cd = A*(1-exp(-1/A)), where A = 4tau(Nmax/3600)

where:

• tau = Heat Pump Time Constant from parent or default 60s
• Nmax = Maximum Cycling Rate from parent or default 2.5/hr

[mjwitte]

@nealkruis IDD changes look good, except for \min-fields. Some have comments below, but every object with new or deleted fields should be double-checked. min-fields should extend at least to the last field with a default.

[mjwitte]

Does Coil:Cooling:DX:VariableSpeed need a bump on \min-fields?

[nealkruis]

It's currently 31, but after the 90.1 metric update the last default field is N93. The prior alpha field is A45...so maybe it should be 93+45=138?

[rraustad]

A13 is a required field. min-fields = 36. This includes these 3 new fields and the 2 recently added 2017/2023 reference fan power fields.

[mjwitte]

Yeah, there's exceptions to the rule, min-fields should also cover the last required-field, and if there are defaulted fields in extensible groups (or pseudo-extensible like this one), min-fields should stop at the last required group, in this case the Speed 1 inputs. So, @rraustad is correct, 36.

[mjwitte]

This needs to extend all the way to the end of the object now, needs to include the last defaulted field.

[mjwitte]

Bumping this by one doesn't really do anything for this object. Either extend to include the new field at the end (to help make it visible in idfs) or leave as-is.

[mjwitte]

This object needs a min-fields.

[Myoldmopar]

I guess this one is still needed.

[mjwitte]

Reduce min-fields?

[nealkruis]

It appears neither of the WaterToAirHeatPump:EquationFit objects had \min-fields before. I can add them.

[nealkruis]

@mjwitte / @rraustad:

1. Docs are complete and ready for review
2. I believe I've addressed the min-fields issues in the IDD
3. There is one integration test failure in Linux debug coverage that seems unrelated. I can't reproduce in my Mac debug build. I merged develop and pushed to see if it's still there.
4. Let me know if you need any more details regarding transition rules

[Myoldmopar]

@nealkruis @RichSee @mjwitte

OK, I think I got things mostly worked out. To catalog exactly what happened:

• 90.1 related changes were added to several coils, including WAHP, in 90.1 metrics - EER/IEER #9756. It turns out the WAHP objects should not have been included. But they were added, including changes to code, docs, example files, tests, and transition rules.
• @nealkruis noticed that they should not have been added, but we asked @RichSee to hold off from doing any reverting just yet, as @nealkruis had some other work to do with the work in this current PR.
• I was helping @nealkruis with the transition rules here in this PR and realized the overlap was substantial on two of the coils. There were only two possible paths forward:
  • Wrap up @nealkruis transition code, integrating tightly with the existing WAHP transition rules. This would lead to needing to then "disintegrate" the transition rules in a following PR.
  • Just rip out the WAHP changes here in this existing PR and only do this once. This is what I chose.

This made this PR a bit beefier than expected, but it was already much bigger of an effort than I thought, so it's fine. Things that need to be done now that I have it "in place":

• @RichSee please give my changes a sanity check. I know there are other changes in this PR, and it is pretty large, but if you can look over it, that would be awesome.
• @nealkruis obviously, you'll need to look over it pretty closely. I don't think I did anything that affected your functional changes in this PR, but still...
• Transition needs to be tested heavily. These transition rules are going to affect many, many user files and they need to be very robust. They are currently not robust enough. One part of this will be me transitioning up every 23.1 example file and checking for issues/diffs once they are converted to 23.2.
• Transition code needs to simply be reviewed as well. Just from an inspection, @mjwitte could probably point out one or two dozen things wrong or protections that need to be added, or inefficiencies to eliminate.
• Transition rules need to be cleaned up. In the original 90.1 PR, the transition rule text was in the wrong rules file, so I moved it over to the 23.2 file, and now some of it needs integration and just a general cleanup.

That's my known list, which is likely to grow, but I need to step away from it for now. If we need to have a discussion, I'm happy to set one up for Monday, just let me know.

[nealkruis]

@Myoldmopar, this looks good to me! Thanks for taking care of all the transition mechanics here!

[Myoldmopar]

OK, so there were a few failing tests, and I have them fixed up locally, about to push. There are diffs showing up on CI, but once I eliminate the failing unit tests, I think they are the exact same diffs that were present on earlier commits of this branch. I will run diffs again locally comparing first to develop (which should reproduce diffs here), then to an earlier version of this branch (which should have none).

[Myoldmopar]

I fixed the one remaining failing unit test, so there should only be diffs from this point on. I ran regressions locally against develop:

• when reporting normally (hourly), I am seeing what Decent is showing here, which is like 30 files showing big mathdiffs and 5 with small mathdiffs. One of the files showing big diffs in plant are like this, so not actually big:

• when reporting on an annual basis, there are no mathdiffs, big or small, which is great. The text diffs appear to be just tiny changes in warmup convergence and tiny changes to values in the error file during warning outputs.

So then I compared the current HEAD of the branch against a previous commit in the branch. I checked out cdf2cf5 and pulled in develop so that the only differences are the changes I have made to clean out WSHP fields. Thankfully, the only diffs are table diffs - even with hourly outputs. 🎈 🎉 Yay, it seems I didn't break anything 👍 🎈 🎉

OK, this needs scrutinizing and still a few things per the checklist comment above, but I think it's close now.

[mjwitte]

@Myoldmopar A few comments on this complex transition.

[mjwitte]

You can use GetNumObjectsFound('AIRLOOPHVAC:UNITARYSYSTEM') etc to get a precise count for any object type.

[Myoldmopar]

Thanks, updated it to allocate to the sum of the count of the relevant objects.

[mjwitte]

These should all be +3? e.g. >=28, 38, etc.

[Myoldmopar]

OK yes, good call! Since I added the three above to CurArgs, these now need to go higher. Updated!

[mjwitte]

Where does ActuallyCreateCurve get used? I think you could use that here to skip incrementing NumCoilLatentStuff (and skip the other assignments here) for a parent object that doesn't have an applicable coil type.

[Myoldmopar]

Hmm, yeah, it's supposed to have been used in the parent objects below to skip creating the curve. Is it OK to leave the assignments the way they are here in the preprocessor, but skip the curve creation below? Or do you think we should change it here in the preprocessing section. At the time of adding that flag, I was still a bit confused about when we would or wouldn't need to generate this new curve, so maybe it's simpler than I am making it out to be.

[Myoldmopar]

Anyway, I added an

IF (.NOT. CoilLatentStuff(Num3)%ActuallyCreateCurve) CYCLE

block to each parent object for now. I'm open to updating it.

[mjwitte]

So, this field goes away and the existing value doesn't get used for anything? Should probably add a warning to the audit file? Actually this whole thing deserves a warning to the audit file and perhaps even a preprocessor warning that shows in the EnergyPlus output. I can find an example of that if need be.

[Myoldmopar]

Yeah a warning is a good idea, and could you take a peek at the ExpandObject changes when you get a chance?

[mjwitte]

ExpandObject changes look good. I'm not sure we have testing coverage for every parent/coil combination, but there should be enough.

[mjwitte]

Instead of READ, it's somewhat safer to use
latentNmax = ProcessNumber(ICoilLatentStuff(Num3)%cMaxCyclingRate,ErrFlag)

[Myoldmopar]

I knew there was a better way, updating it now.

[mjwitte]

Is this safe if the incoming object has <27 fields? I think you need to copy 1:CurArgs here.

[Myoldmopar]

Hmm...so how do we know whether to append them? Is this OK?

OutArgs(1:CurArgs)=InArgs(1:CurArgs)
IF (CurArgs > 27) THEN
    OutArgs(28) = CoilLatentStuff(Num3)%NewCurveName
    OutArgs(29) = CoilLatentStuff(Num3)%cMaxCyclingRate
    OutArgs(30) = CoilLatentStuff(Num3)%cHeatPumpTimeConst
    OutArgs(31) = CoilLatentStuff(Num3)%cHPDelayTime
    CurArgs = 31
END IF

If there is less than 27 input, then I'm not sure what the outcome would be.

[mjwitte]

Good question, I believe the fields in between will be written as blank, but we'll have to test one to be sure.

[Myoldmopar]

I ran transition on the entire set of 23.1 example files. Found a couple issues and fixed them, and everything transitioned successfully. I am now running the full set of example files from both this branch (manually transitioned) and then the auto-transitioned files next. I'm grabbing the run status (exit code) from each, as well as the ERR file contents (cleaned of timestamps). I'll compare those and report back tonight or first thing in the morning. Assuming they are producing the same ERR file contents, that should be indicative that my transition code is properly getting 23.1 files up to where we expect them to be for 23.2. 🤞

[Myoldmopar]

@nealkruis I noticed that in the example files, the new PLF curve appears to (at least in 5ZoneWLHPPlantLoopTower) be set to the existing linear curve:

  Curve:Quadratic,
    HPACCOOLPLFFPLR,         !- Name
    0.75,                    !- Coefficient1 Constant
    0.25,                    !- Coefficient2 x
    0.0,                     !- Coefficient3 x**2
    0.0,                     !- Minimum Value of x
    1.0;                     !- Maximum Value of x

However, when my transition code is running, it mines data from the parent unitary WAHP:

• MaxCyclingRate = IDFRecords(Num)%Numbers(4) = 2.5
• HeatPumpTimeConst = IDFRecords(Num)%Numbers(5) = 60
• FractionOnCycle = IDFRecords(Num)%Numbers(6) = 0.01
• HPDelayTime = IDFRecords(Num)%Numbers(7) = 60

Then during the main transition loop, I create a new linear curve as such:

• latentTau = CoilLatentStuff(Num3)%cHeatPumpTimeConst = 60
• latentNmax = CoilLatentStuff(Num3)%cMaxCyclingRate = 2.5
• latentA = 4 * latentTau * (latentNmax / 3600.0) = 0.1667
• latentCd = latentA * (1 - EXP(-1 / latentA)) = 0.1663
• Curve A value = 1 - latentCd = 0.8337
• Curve B value = latentCd = 0.1663

So my curve is 0.8337x + 0.1663. The example files in the branch right now have 0.75x + 0.25. I hoped that cleaning this up would eliminate the diffs entirely, but they don't seem to. I'm currently still diagnosing some transition issues. In the meantime, could you comment on this stuff?

[Myoldmopar]

ALLLLLRIGHT I think this is ready.

• I took the full set of 23.1 example files and transitioned them up with the Fortran transition program. Some transitions failed, so I had to iterate a couple times on that, but eventually got them all transitioning properly.
• I then ran EnergyPlus on every example file, both the ones manually converted living in this branch, and the ones I programmatically transitioned. I compared the exit codes and the ERR files. There were some failures that required changes to the transition code. I iterated on this a couple times, but eventually got the files running great and am happy with it. The composite ERR file diff only shows some changes related to warning message numerics, which could be due to me creating PLF curves from actual inputs rather than using the 0.75x+0.25 as mentioned above.

OK -- I think it's worth a once-over on this whole PR set of changes and then maybe move this into the merge queue. @nealkruis @mjwitte @rraustad @RichSee if you have anything further please let me know.

[Myoldmopar]

OK, I looked it over one more time. I am good with it except one IDD comment from @mjwitte . Perhaps the IDD in general is worth another look after the 90.1 changes were reverted from it. Either way I think this is very close.

[Myoldmopar]

Docs shuffling fields around and updating sample inputs, 👍

[Myoldmopar]

IDD changes all appear to be functioning properly, and since they were generally copy/pasted, I don't feel inclined to scrutinize spelling and such. Moving on...

[Myoldmopar]

I guess this one is still needed.

[Myoldmopar]

Code changes seem to be working well based heavily on CI results and local testing.

[Myoldmopar]

These changes should have gone in the 23.1 to 23.2 file, so I put it in over there.

[Myoldmopar]

It would be good for someone to read through this, but not mandatory before merge. The text could be improved post-IO-freeze if absolutely necessary.

[Myoldmopar]

I'll resolve conflicts one more time here. If I don't hear any concerns from anyone in the near term, I'm going to merge this in. Thanks all!

[Myoldmopar]

OK! Goodbye PR #10043!

[rraustad]

I don't recall discussing this Fan Delay Time input and how it relates to the coil versus the parent. I'm not really sure where this belongs at this point so I'll assume this is correctly accounted for in these changes. I scanned through the rest of the changes and this is the only thing that stands out.

[nealkruis]

@rraustad, the fan delay is a strange input that is used only for the latent degradation model in these coils. I'm not sure why it's not used in other the air-to-air DX coils. Eventually it would be good to have a base coil class that all these coils inherit from that is the one and only place where the latent degradation model is implemented.

[jmarrec]

\required-field is missing here.

EnergyPlus/src/EnergyPlus/WaterToAirHeatPumpSimple.cc

Lines 610 to 620 in f1db7f4

	simpleWAHP.PLFCurveIndex = Curve::GetCurveIndex(state, AlphArray(8)); // convert curve name to number
	if (simpleWAHP.PLFCurveIndex == 0) {
	if (lAlphaBlanks(8)) {
	ShowSevereError(state, format("{}{}=\"{}\", missing", RoutineName, CurrentModuleObject, simpleWAHP.Name));
	ShowContinueError(state, format("...required {} is blank.", cAlphaFields(8)));
	} else {
	ShowSevereError(state, format("{}{}=\"{}\", invalid", RoutineName, CurrentModuleObject, simpleWAHP.Name));
	ShowContinueError(state, format("...not found {}=\"{}\".", cAlphaFields(8), AlphArray(8)));
	}
	ErrorsFound = true;

[jmarrec]

\required-field missing here

EnergyPlus/src/EnergyPlus/WaterToAirHeatPumpSimple.cc

Lines 365 to 375 in f1db7f4

	simpleWAHP.PLFCurveIndex = Curve::GetCurveIndex(state, AlphArray(9)); // convert curve name to number
	if (simpleWAHP.PLFCurveIndex == 0) {
	if (lAlphaBlanks(9)) {
	ShowSevereError(state, format("{}{}=\"{}\", missing", RoutineName, CurrentModuleObject, simpleWAHP.Name));
	ShowContinueError(state, format("...required {} is blank.", cAlphaFields(9)));
	} else {
	ShowSevereError(state, format("{}{}=\"{}\", invalid", RoutineName, CurrentModuleObject, simpleWAHP.Name));
	ShowContinueError(state, format("...not found {}=\"{}\".", cAlphaFields(9), AlphArray(9)));
	}
	ErrorsFound = true;

[jmarrec]

\required-field missing

EnergyPlus/src/EnergyPlus/WaterToAirHeatPump.cc

Lines 390 to 400 in c854ba6

	heatPump.PLFCurveIndex = Curve::GetCurveIndex(state, AlphArray(8)); // convert curve name to number
	if (heatPump.PLFCurveIndex == 0) {
	if (lAlphaBlanks(8)) {
	ShowSevereError(state, format("{}{}=\"{}\", missing", RoutineName, CurrentModuleObject, heatPump.Name));
	ShowContinueError(state, format("...required {} is blank.", cAlphaFields(8)));
	} else {
	ShowSevereError(state, format("{}{}=\"{}\", invalid", RoutineName, CurrentModuleObject, heatPump.Name));
	ShowContinueError(state, format("...not found {}=\"{}\".", cAlphaFields(8), AlphArray(8)));
	}
	ErrorsFound = true;

[jmarrec]

Same here, \required-field

EnergyPlus/src/EnergyPlus/WaterToAirHeatPump.cc

Lines 637 to 647 in c854ba6

	heatPump.PLFCurveIndex = Curve::GetCurveIndex(state, AlphArray(8)); // convert curve name to number
	if (heatPump.PLFCurveIndex == 0) {
	if (lAlphaBlanks(8)) {
	ShowSevereError(state, format("{}{}=\"{}\", missing", RoutineName, CurrentModuleObject, heatPump.Name));
	ShowContinueError(state, format("...required {} is blank.", cAlphaFields(8)));
	} else {
	ShowSevereError(state, format("{}{}=\"{}\", invalid", RoutineName, CurrentModuleObject, heatPump.Name));
	ShowContinueError(state, format("...not found {}=\"{}\".", cAlphaFields(8), AlphArray(8)));
	}
	ErrorsFound = true;

[jmarrec]

N7 + A8 with last one required => \min-fields 15