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

Wrap Coil:Cooling:DX:SingleSpeed:ThermalStorage #5059

Merged
merged 61 commits into from
Mar 27, 2024
Merged

Conversation

joseph-robertson
Copy link
Collaborator

@joseph-robertson joseph-robertson commented Dec 8, 2023

Pull request overview

  • Fixes Wrap Coil:Cooling:DX:SingleSpeed:ThermalStorage #5055.
  • Questions / issues:
    • Inconsistencies in autosizable vs autocalculatable; change openstudio idd to be consistent? (Appears perhaps E+ doesn't make distinction between "autosize" and "autocalculate"?)
      • Update: I changed all autocalculatable to autosizable.
    • Ctor should follow RetailPackagedTESCoil.idf? Or a mix of the idf values and idd defaults?
      • Update: Ctor generally follows idd defaults, with a few exceptions where it uses values found in RetailPackagedTESCoil.idf.
    • Make all non curve fields required so that, even when Mode=No, the fields are pre-populated in case Mode is switched to Yes?
      • Update: No, not taking this approach. All modes are set to No in the Ctor and no curves are populated; it is up to the user to provide all the required curves for a given mode when set to Yes.

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: Wrap Coil:Cooling:DX:SingleSpeed:ThermalStorage OpenStudio-resources#206
  • 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 Enhancement Request component - HVAC component - Model Pull Request - Ready for CI This pull request if finalized and is ready for continuous integration verification prior to merge. IDDChange labels Dec 8, 2023
@joseph-robertson joseph-robertson self-assigned this Dec 8, 2023
@joseph-robertson
Copy link
Collaborator Author

@shorowit pointed out that it's not always appropriate to use idf testfile values.
@DavidGoldwasser pointed out that there may be helpful curve information on other DX coil objects.

@jmarrec
Copy link
Collaborator

jmarrec commented Jan 29, 2024

Make all non curve fields required so that, even when Mode=No, the fields are pre-populated in case Mode is switched to Yes?

I'm kinda torn on this one. There are three options:

  • Indeed make them required, do not translate them if the choice field is No
    • Pros: easier for the user
    • Cons: that's 5 extra curves for each of these... model bloats
  • Make a model-time bool validityCheck() method, and call that at the beginning of the FT or something similar (this is what's done for the ElectricLoadCenterDistribution, since dependending on some choice fields you must have an inverter or not for eg). Then issue a loud warn, and do not translate it.
    • Pros: identifies the issue (provided the user actually reads the warnings)
    • Cons: could potentially not be translated without the user realizing
  • Leave them optional, check nothing, let E+ crash (I assume E+ actually crashes)
    • Pros: implementation is easier
    • Cons: problem is detected much later

Worth noting that I doubt this object will be commonly used by people, so factor that in.

@kbenne Can you please advise here?

@joseph-robertson
Copy link
Collaborator Author

My vote is leave the curves as optionals (so either bullet 2 or 3); it doesn't look the one testfile on the E+ repo with Coil:Cooling:DX:SingleSpeed:ThermalStorage has all the curve fields populated, so what would we set them to?

@joseph-robertson
Copy link
Collaborator Author

I don't see where ElectricLoadCenterDistribution's validityCheck even gets called?

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.

Thanks for the changes @joseph-robertson

I think there are a couple of loose ens to tie up still, but it's almost there!

resources/model/OpenStudio.idd Outdated Show resolved Hide resolved
src/model/CoilCoolingDXSingleSpeedThermalStorage.cpp Outdated Show resolved Hide resolved
Comment on lines +1980 to +1983
ok = setAncillaryElectricPower(0.0);
OS_ASSERT(ok);
ok = setColdWeatherOperationAncillaryPower(0.0);
OS_ASSERT(ok);
Copy link
Collaborator

Choose a reason for hiding this comment

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

(now that I've requested it, I hope we don't find out later that E+ treats blank and zero differently... sometimes that happens)

src/model/CoilCoolingDXSingleSpeedThermalStorage.cpp Outdated Show resolved Hide resolved
EXPECT_EQ(0u, rmed.size());
ASSERT_TRUE(unitary.coolingCoil());
EXPECT_EQ(dx, unitary.coolingCoil().get());
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe test you can setCoolingCoil with another type of Unitary sys then?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Same: test with the ZoneHVACComponents.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

setCoolingCoil for these system types should throw? Do we have examples/precedents for doing this?

EXPECT_EQ(2u, model.getConcreteModelObjects<AirLoopHVACUnitarySystem>().size());
EXPECT_EQ(2u, model.getConcreteModelObjects<CoilCoolingDXSingleSpeedThermalStorage>().size());
EXPECT_EQ(dx, unitary.coolingCoil().get());
EXPECT_NE(dx, unitaryClone.coolingCoil().get());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Need test to check what happens when you clone to another model. And when removing (are curves dragged / removed?)

Comment on lines 321 to 323
} else if (_coolingCoil->iddObject().type() == IddObjectType::Coil_Cooling_DX_SingleSpeed_ThermalStorage) {
_coolingCoil->setString(Coil_Cooling_DX_SingleSpeed_ThermalStorageFields::EvaporatorAirInletNodeName, coolingCoilInletNodeName);
_coolingCoil->setString(Coil_Cooling_DX_SingleSpeed_ThermalStorageFields::EvaporatorAirOutletNodeName, coolingCoilOutletNodeName);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Either we can assign it to a PTAC, PTHP, then we should have a containingZoneHVACComponent implemented in model, or this should be removed.

Either way, looks like we need model tests indeed to check what this can or cannot be assigned to.

@jmarrec
Copy link
Collaborator

jmarrec commented Mar 5, 2024

@joseph-robertson One failing test

1056: [ RUN      ] ModelFixture.CoilCoolingDXSingleSpeedThermalStorage_clone
1056: /srv/jenkins/openstudio/docker-volumes/ubuntu-2004/src/model/test/CoilCoolingDXSingleSpeedThermalStorage_GTest.cpp:992: Failure
1056: Expected equality of these values:
1056:   0
1056:   model.getModelObjects<Curve>().size()
1056:     Which is: 3
1056: /srv/jenkins/openstudio/docker-volumes/ubuntu-2004/src/model/test/CoilCoolingDXSingleSpeedThermalStorage_GTest.cpp:993: Failure
1056: Expected equality of these values:
1056:   0
1056:   model.getConcreteModelObjects<CurveBiquadratic>().size()
1056:     Which is: 1
1056: /srv/jenkins/openstudio/docker-volumes/ubuntu-2004/src/model/test/CoilCoolingDXSingleSpeedThermalStorage_GTest.cpp:994: Failure
1056: Expected equality of these values:
1056:   0
1056:   model.getConcreteModelObjects<CurveQuadratic>().size()
1056:     Which is: 1
1056: /srv/jenkins/openstudio/docker-volumes/ubuntu-2004/src/model/test/CoilCoolingDXSingleSpeedThermalStorage_GTest.cpp:995: Failure
1056: Expected equality of these values:
1056:   0
1056:   model.getConcreteModelObjects<CurveTriquadratic>().size()
1056:     Which is: 1
1056: [  FAILED  ] ModelFixture.CoilCoolingDXSingleSpeedThermalStorage_clone (10 ms)
1056: [----------] 1 test from ModelFixture (11 ms total)
1056: 
1056: [----------] Global test environment tear-down
1056: [==========] 1 test from 1 test suite ran. (11 ms total)
1056: [  PASSED  ] 0 tests.
1056: [  FAILED  ] 1 test, listed below:
1056: [  FAILED  ] ModelFixture.CoilCoolingDXSingleSpeedThermalStorage_clone

@ci-commercialbuildings
Copy link
Collaborator

ci-commercialbuildings commented Mar 12, 2024

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.

Thanks for the work @joseph-robertson ! Tests are passing now, this looks good to go

@jmarrec jmarrec merged commit bf61a99 into develop Mar 27, 2024
3 of 6 checks passed
@jmarrec jmarrec deleted the coil-thermal-storage branch March 27, 2024 15:26
jmarrec added a commit that referenced this pull request May 7, 2024
@jmarrec jmarrec mentioned this pull request May 7, 2024
19 tasks
jmarrec added a commit that referenced this pull request May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component - HVAC component - Model Enhancement Request 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.

Wrap Coil:Cooling:DX:SingleSpeed:ThermalStorage
3 participants