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

ZoneHVAC:*: new Design Specification Multispeed Object fields #4964

Draft
wants to merge 26 commits into
base: develop
Choose a base branch
from

Conversation

joseph-robertson
Copy link
Collaborator

@joseph-robertson joseph-robertson commented Sep 11, 2023

Pull request overview

Support new fields "Design Specification Multispeed Object Type" and "Design Specification Multispeed Object Name" for:

  • ZoneHVAC:WaterToAirHeatPump
  • ZoneHVAC:TerminalUnit:VariableRefrigerantFlow

Also, add:

  • UnitarySystemPerformanceMultispeed FT support for CoilXXXDXVariableSpeed and CoilXXXWaterToAirHeatPumpVariableSpeedEquationFit (FT for AirLoopHVACUnitarySystem supports these coil types but FT for UnitarySystemPerformanceMultispeed didn't previously)
  • a new UnitarySystemPerformanceMultispeed_GTest.cpp FT test file for translating UnitarySystemPerformanceMultispeed on AirLoopHVACUnitarySystem, ZoneHVACWaterToAirHeatPump, and ZoneHVACTerminalUnitVariableRefrigerantFlow

Pull Request Author

  • Model API Changes / Additions
  • Any new or modified fields have been implemented in the EnergyPlus ForwardTranslator (and ReverseTranslator as appropriate)
  • Model API methods are tested (in src/model/test)
  • EnergyPlus ForwardTranslator Tests (in src/energyplus/Test)
  • If a new object or method, added a test in NREL/OpenStudio-resources: Add Link
  • If needed, added VersionTranslation rules for the objects (src/osversion/VersionTranslator.cpp)
  • Verified that C# bindings built fine on Windows, partial classes used as needed, etc.
  • All new and existing tests passes
  • If methods have been deprecated, update rest of code to use the new methods

Labels:

  • If change to an IDD file, add the label IDDChange
  • If breaking existing API, add the label APIChange
  • If deemed ready, add label Pull Request - Ready for CI so that CI builds your PR

Review Checklist

This will not be exhaustively relevant to every PR.

  • Perform a Code Review on GitHub
  • Code Style, strip trailing whitespace, etc.
  • All related changes have been implemented: model changes, model tests, FT changes, FT tests, VersionTranslation, OS App
  • Labeling is ok
  • 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

@joseph-robertson joseph-robertson added Pull Request - Ready for CI This pull request if finalized and is ready for continuous integration verification prior to merge. IDDChange labels Sep 11, 2023
@joseph-robertson joseph-robertson added this to the OpenStudio SDK 3.7.0 milestone Sep 11, 2023
@joseph-robertson joseph-robertson self-assigned this Sep 11, 2023
@joseph-robertson joseph-robertson mentioned this pull request Sep 11, 2023
26 tasks
resources/model/OpenStudio.idd Outdated Show resolved Hide resolved
Comment on lines +82 to +84
if (auto designSpec = designSpecificationMultispeedObject()) {
wahpClone.setDesignSpecificationMultispeedObject(designSpec->clone(model).cast<UnitarySystemPerformanceMultispeed>());
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, AirLoopHVACUNitarySystem already clones / remove, so let's do the same

src/model/ZoneHVACWaterToAirHeatPump.cpp Outdated Show resolved Hide resolved
@@ -432,6 +434,18 @@ namespace energyplus {
}
}

// Design Specification Multispeed Object Name
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see any FT testing for this one?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@jmarrec I didn't get too far on this PR yet. I sort of got tripped up on FT. Appears that for AirLoopHVACUnitarySystem, it FTs the UnitarySystemPerformanceMultispeed object, but also creates one if one doesn't exist. Under what conditions should we create one for ZoneHVACWaterToAirHeatPump and ZoneHVACTerminalUnitTVariableRefrigerantFlow?

Copy link
Collaborator

Choose a reason for hiding this comment

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

AirLoopHVACUnitarySystem I believe throws if you use multispeed / variable speed coils without a UnitarySystemPerformanceMultispeed object, hence why the FT creates one if needed.

I'm not 100% sure whether the VRF TU and the ZoneHVAC:WaterToAirHeatPump have the same behavior now. If so, we should create one in FT. In that case, should move the default creation to a helper function so we can reuse in other modules.

Copy link
Collaborator

Choose a reason for hiding this comment

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

oh, that's quite confusing, but it seems that this applies not to the coils but to the fans. The original NFP proposed a new object "FanPerformance:Multispeed", but apparently UnistarySystemPerformance:Multispeed was kept.

https://github.com/NREL/EnergyPlus/blob/develop/design/FY2023/NFP_MultispeedFans.md

My understanding is that this is purely optional, regardless of the coils you use for the ZoneHVAC:WAHP or ZoneHVAC:TU:VRF objects. We should probably make some dummy IDF tests to confirm. I'll ask the E+ dev team right now too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@jmarrec I stubbed UnitarySystemPerformance FT to handle additional coil types CoilXXXWaterToAirHeatPumpEquationFit, CoilXXXDXVariableRefrigerantFlow, and CoilXXXDXVariableRefrigerantFlowFluidTemperatureControl. But I'm a little unclear on how to determine NumberofSpeedsforXXX. The speeds should not correspond to the coils, but to the supply fan? Are we going to need to create a new setter method for speeds on the UnitarySystemPerformance object?

Copy link
Collaborator

Choose a reason for hiding this comment

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

My earlier post was misleading I think. The object varies the fan airflow in conjunction with the heating/cooling capacity.
So Number of Speeds for Heating should match the heating coil number of speeds, when the coil does have speeds.

I don't think we need to create a UnitarySystemPerformance object for VRF / ZoneHVACWaterToAirHeatPump if they don't throw when one isn't present. In that case, we're just left translating whatever the user chose to specify.

Also, take VRFMultispeedFan.idffor eg. You have one ZoneHVAC:TerminalUnit:VariableRefrigerantFlow (TU1) object with some COIL:Cooling:DX:VariableRefrigerantFlow and COIL:Heating:DX:VariableRefrigerantFlow. These coils do not have speed-related fields. Yet there is a UnitarySystemPerformance assigned to it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm looking at VRFMultispeedFan.idf...

  UnitarySystemPerformance:Multispeed,
    Sys 1 WaterToAirHP System MultiSpeed Performance,  !- Name
    2,                       !- Number of Speeds for Heating
    2,                       !- Number of Speeds for Cooling

Where does the "2" come from? Should this value be hardcoded when coil object types are COIL:*:DX:VariableRefrigerantFlow?

Copy link
Collaborator

Choose a reason for hiding this comment

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

That is arbitrary I believe.

Comment on lines 273 to 274
EXPECT_TRUE(idf_hp.isEmpty(ZoneHVAC_WaterToAirHeatPumpFields::DesignSpecificationMultispeedObjectType));
EXPECT_TRUE(idf_hp.isEmpty(ZoneHVAC_WaterToAirHeatPumpFields::DesignSpecificationMultispeedObjectName));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please actually assign one and check that it's all good.

@joseph-robertson joseph-robertson marked this pull request as ready for review September 26, 2023 23:28
Copy link
Collaborator

@jmarrec jmarrec 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 a tough one and I'm not sure I completely understand how this object is supposed to be used when not in a AirLoopHVAC:UnitarySystem. Maybe we should just punt on this new feature until a bit later after we've merged 23.2.0-IOFreeze to develop?

This does not enable using the object with VRF objects at least (eg, VRFMultispeedFan.idf is not reproducible). Seems like it'd work for the ZoneHVACWaterToAirHeatPump in most cases though.

For VRF we'd need to expose the Number of Speeds for <Heating|Cooling> fields nor the <Heating|Cooling> Speed <N> Supply Air Flow Ratio fields. Which may be problematic for other objects because that means we need to handle the case where what the user manually entered is inconsistent with the VSD coil's number of Speeds.

Comment on lines +78 to +94
if (unitarySystems.size() == 1) {
AirLoopHVACUnitarySystem& unitarySystem = unitarySystems[0];
heatingCoil = unitarySystem.heatingCoil();
coolingCoil = unitarySystem.coolingCoil();
}

if (tuVRFs.size() == 1) {
ZoneHVACTerminalUnitVariableRefrigerantFlow& tuVRF = tuVRFs[0];
heatingCoil = tuVRF.heatingCoil();
coolingCoil = tuVRF.coolingCoil();
}

if (hps.size() == 1) {
ZoneHVACWaterToAirHeatPump& hp = hps[0];
heatingCoil = hp.heatingCoil().optionalCast<HVACComponent>();
coolingCoil = hp.coolingCoil().optionalCast<HVACComponent>();
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if (unitarySystems.size() == 1) {
AirLoopHVACUnitarySystem& unitarySystem = unitarySystems[0];
heatingCoil = unitarySystem.heatingCoil();
coolingCoil = unitarySystem.coolingCoil();
}
if (tuVRFs.size() == 1) {
ZoneHVACTerminalUnitVariableRefrigerantFlow& tuVRF = tuVRFs[0];
heatingCoil = tuVRF.heatingCoil();
coolingCoil = tuVRF.coolingCoil();
}
if (hps.size() == 1) {
ZoneHVACWaterToAirHeatPump& hp = hps[0];
heatingCoil = hp.heatingCoil().optionalCast<HVACComponent>();
coolingCoil = hp.coolingCoil().optionalCast<HVACComponent>();
}
if (unitarySystems.size() == 1) {
AirLoopHVACUnitarySystem& unitarySystem = unitarySystems.front();
heatingCoil = unitarySystem.heatingCoil();
coolingCoil = unitarySystem.coolingCoil();
} else if (tuVRFs.size() == 1) {
ZoneHVACTerminalUnitVariableRefrigerantFlow& tuVRF = tuVRFs.front();
heatingCoil = tuVRF.heatingCoil();
coolingCoil = tuVRF.coolingCoil();
} else { // if (hps.size() == 1)
ZoneHVACWaterToAirHeatPump& hp = hps.front();
heatingCoil = hp.heatingCoil().optionalCast<HVACComponent>();
coolingCoil = hp.coolingCoil().optionalCast<HVACComponent>();
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because of the if (((unitarySystems.size() + tuVRFs.size() + hps.size()) != 1)) { line above, yes, I think we'd be able to collapse to your suggestion.

Comment on lines +104 to +110
} else if (heatingCoil->iddObjectType() == openstudio::IddObjectType::OS_Coil_Heating_DX_VariableSpeed) {
auto heatingCoilDXVarspeed = heatingCoil->cast<CoilHeatingDXVariableSpeed>();
sysPerf.setInt(UnitarySystemPerformance_MultispeedFields::NumberofSpeedsforHeating, heatingCoilDXVarspeed.speeds().size());
} else if (heatingCoil->iddObjectType() == openstudio::IddObjectType::OS_Coil_Heating_WaterToAirHeatPump_VariableSpeedEquationFit) {
auto heatingCoilWaterToAirHeatingVar = heatingCoil->cast<CoilHeatingWaterToAirHeatPumpVariableSpeedEquationFit>();
sysPerf.setInt(UnitarySystemPerformance_MultispeedFields::NumberofSpeedsforHeating, heatingCoilWaterToAirHeatingVar.speeds().size());
} else { // e.g., CoilHeatingWaterToAirHeatPumpEquationFit, CoilHeatingDXVariableRefrigerantFlow, CoilHeatingDXVariableRefrigerantFlowFluidTemperatureControl
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you run an AirLoopHVACUnitarySystem (or a ZoneHVACWAHP) with these coils and no UnitarySystemPerformance object, does E+ throw?

Just curious if you tested it, and same question for the below cooling coils below

Comment on lines +438 to +447
if (boost::optional<UnitarySystemPerformanceMultispeed> designSpecificationMultispeedObject = modelObject.designSpecificationMultispeedObject()) {
boost::optional<IdfObject> _unitarySystemPerformance = translateAndMapModelObject(designSpecificationMultispeedObject.get());

if (_unitarySystemPerformance && _unitarySystemPerformance->name()) {
idfObject.setString(ZoneHVAC_TerminalUnit_VariableRefrigerantFlowFields::DesignSpecificationMultispeedObjectType,
_unitarySystemPerformance->iddObject().name());
idfObject.setString(ZoneHVAC_TerminalUnit_VariableRefrigerantFlowFields::DesignSpecificationMultispeedObjectName,
_unitarySystemPerformance->name().get());
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is pointless here for the VRF object, because you don't have a way to set the number of Speeds during Heating/Cooling, and the VRF coils do not bear the requested information to autofill them.

Given what we see in VRFMultispeedFan.idf that means:

  • We actually need to expose the number of heating/cooling speeds
  • We also need the extensible portion <Heating|Cooling> Speed <N> Supply Air Flow Ratio
  • We probably need to throw or overwrite when writing for a coil that has an actual number of speeeds
  • We probably need to reach out to the E+ devs responsible for this feature to understand it fully. I'm quite fuzzy on how this is used with coils that don't have an actual number of speeds

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Couple of things:
(1) I had noticed the "gap" on the heating/cooling speeds - that's why I had posed the question "Are we going to need to create a new setter method for speeds on the UnitarySystemPerformance object?" above.
(2) I thought you had said that the unitary system performance speeds in VRFMultispeedFan.idf were arbitrary?

But generally speaking, yes, I agree that we should probably punt on this new feature until later.

Comment on lines +361 to +369
if (boost::optional<UnitarySystemPerformanceMultispeed> designSpecificationMultispeedObject = modelObject.designSpecificationMultispeedObject()) {
boost::optional<IdfObject> _unitarySystemPerformance = translateAndMapModelObject(designSpecificationMultispeedObject.get());

if (_unitarySystemPerformance && _unitarySystemPerformance->name()) {
idfObject.setString(ZoneHVAC_WaterToAirHeatPumpFields::DesignSpecificationMultispeedObjectType,
_unitarySystemPerformance->iddObject().name());
idfObject.setString(ZoneHVAC_WaterToAirHeatPumpFields::DesignSpecificationMultispeedObjectName, _unitarySystemPerformance->name().get());
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unlike VRF, there are a varierty of coils when we can autofill the Number of Speeds for <Heating|Cooling> fields nor the <Heating|Cooling> Speed <N> Supply Air Flow Ratio.
One, or maybe the only one, where we cannot is the CoilHeatingWaterToAirHeatPumpEquationFit, and perhaps that's fine. I'm not 100% sure

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
IDDChange Pull Request - Ready for CI This pull request if finalized and is ready for continuous integration verification prior to merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants