Addresses #4941, wrap DistrictHeating:Steam #4954

joseph-robertson · 2023-08-30T16:35:38Z

Pull request overview

Fixes E+ 23.2.0: Wrap DistrictHeating:Steam #4941.
Also adds support for "Capacity Fraction Schedule Name" field on:
- DistrictCooling
- DistrictHeating:Water
- DistrictHeating:Steam

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

src/model/ScheduleTypeRegistry.cpp

src/model/test/DistrictCooling_GTest.cpp

src/model/test/DistrictHeatingSteam_GTest.cpp

src/model/test/DistrictHeating_GTest.cpp

jmarrec · 2023-09-12T13:09:14Z

src/model/DistrictHeating.hpp

@@ -13,6 +13,8 @@ namespace openstudio {

 namespace model {

+  class Schedule;
+
  namespace detail {

    class DistrictHeating_Impl;


I am very tempted to rename the class to DistrictHeatingWater and IDD Object too, to avoid the same fiasco as WaterHeaterHeatPump, which should be WaterHeatHeatPumpPumped since the WaterHeaterHeatPumpWrappedCondenser was added later.

We can provide an alias for backward compatibility... Same as what we did for AirTerminalSingleDuctUncontrolled => AirTerminalSingleDuctConstantVolumeNoReheat (it's perhaps time to remove it now that I think abouve it)

I'm not opposed if there is an alias. Is there a precedent for alias on Model types anywhere else? Certainly we have them for fields/methods, but I'm not sure about types.

I have made that change in #4972. Fully aliased, with a deprecated message in both C++, as well bindings (Ruby and Python)

Yes, we have a precedent, AirTerminalSingleDuctUncontrolled -> AirTerminalSingleDuctConstantVolumeNoReheat. Which we have since 2.7.0 and we should definitely remove now.

https://github.com/NREL/OpenStudio/pull/4972/files#diff-ccd34a0619fa7fdf2219b31ac16a85edbbd5c8fae3459e6fb7bee04d67402aa6

src/energyplus/ForwardTranslator/ForwardTranslateDistrictHeatingSteam.cpp

jmarrec · 2023-09-12T13:11:19Z

src/energyplus/ForwardTranslator.cpp

@@ -3497,6 +3502,7 @@ namespace energyplus {
      IddObjectType::OS_Table_Lookup,
      IddObjectType::OS_DistrictCooling,
      IddObjectType::OS_DistrictHeating,
+      IddObjectType::OS_DistrictHeating_Steam,


Please don't. This object should only be translated if it's on a plant loop. Same for the other District objects for that matter.

I commented out these 3. Is that enough?

jmarrec · 2023-09-12T13:12:32Z

resources/model/OpenStudio.idd

+  A5 ; \field Capacity Fraction Schedule
+       \note Schedule values are multiplied by Nominal Capacity for current capacity
+       \type object-list
+       \object-list ScheduleNames


Required field, ctor set to alwaysOnContinuousSchedule?

Or is it better to follow the API of the two similar objects.

(in fact, DistrictHeating:Water, DistrictCooling, and DistrictHeating:Steam are exactly the same, and E+ uses a common routine to get their input).

@kbenne thoughts?

My gut is make DistrictHeating::capacityFractionSchedule and DistrictHeatingSteam::capacityFractionSchedule since both are new fields and I really hope we can reduce optionals as much as possible. Then I guess the question is do we make this required in the IDD? I think yes, but then we need to do version translation for the existing DistrictHeating type. I suppose the alternative would be to handle it in the API and return a default schedule if the osm does not define one, but I'm guessing that is not a popular option.

I can make them non optional, in the new PR #4972 if we think that's better. VT isn't that complicated.

ci-commercialbuildings · 2023-09-12T18:11:26Z

CI Results for 431293d:

Ubuntu 20.04:
Ubuntu 22.04:
- OpenStudio-3.7.0-alpha+f791bc31ea-Ubuntu-22.04-x86_64.deb
- OpenStudio-3.7.0-alpha+f791bc31ea-Ubuntu-22.04-x86_64.tar.gz
Darwin:
- OpenStudio-3.7.0-alpha+95d6f7b0f3-Darwin-x86_64.dmg
- OpenStudio-3.7.0-alpha+95d6f7b0f3-Darwin-x86_64.tar.gz
Windows:
- OpenStudio-3.7.0-alpha+b87e73b163-Windows.exe
- OpenStudio-3.7.0-alpha+b87e73b163-Windows.tar.gz
Code Coverage:

kbenne · 2023-09-19T16:31:56Z

resources/model/OpenStudio.idd

+  A5 ; \field Capacity Fraction Schedule
+       \note Schedule values are multiplied by Nominal Capacity for current capacity
+       \type object-list
+       \object-list ScheduleNames


My gut is make DistrictHeating::capacityFractionSchedule and DistrictHeatingSteam::capacityFractionSchedule since both are new fields and I really hope we can reduce optionals as much as possible. Then I guess the question is do we make this required in the IDD? I think yes, but then we need to do version translation for the existing DistrictHeating type. I suppose the alternative would be to handle it in the API and return a default schedule if the osm does not define one, but I'm guessing that is not a popular option.

kbenne · 2023-09-19T16:33:10Z

src/model/DistrictHeating.hpp

@@ -13,6 +13,8 @@ namespace openstudio {

 namespace model {

+  class Schedule;
+
  namespace detail {

    class DistrictHeating_Impl;


I'm not opposed if there is an alias. Is there a precedent for alias on Model types anywhere else? Certainly we have them for fields/methods, but I'm not sure about types.

jmarrec · 2023-09-19T18:54:23Z

Superseded by #4972 (I merged this PR in it)

@kbenne

…edule) The grand protector of the API has spoken! cf @kbenne 's comment here: #4954 (comment)

joseph-robertson added 5 commits August 30, 2023 09:20

Update idd for water fraction schedule and new district heating steam.

467b13b

Updates for district heating water capacity fraction schedule.

857c073

Add source files for district heating steam.

1f48f28

Add ft files for district heating steam.

bb5c0c1

Formatting.

bb9ef23

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 Aug 30, 2023

joseph-robertson self-assigned this Aug 30, 2023

joseph-robertson mentioned this pull request Aug 29, 2023

Update to EnergyPlus 23.2.0 #4934

Closed

26 tasks

joseph-robertson added 6 commits August 30, 2023 10:07

Source ft file updates for district heating steam.

04eb9c0

Few more updates in hpp files.

78c53dd

Update all district types for capacity fraction schedule.

94c6c24

Fix includes for district cooling.

2f20fb8

Few updates around steam vs district heating steam.

90986ea

Minor typo in new district model tests.

35666bd

joseph-robertson mentioned this pull request Aug 30, 2023

Wrap DistrictHeating:Steam NREL/OpenStudio-resources#200

Closed

23 tasks

jmarrec reviewed Sep 12, 2023

View reviewed changes

joseph-robertson added 5 commits September 12, 2023 08:40

Update schedule type registry.

74b1049

Update ft for all district objects.

42296ae

Add nominal capacity in model tests.

709a375

Create new ft tests for all district objects.

059f4d0

Few model test typos and formatting issues.

431293d

jmarrec mentioned this pull request Sep 19, 2023

V23.2.0-IOFreeze - wrap DistrictHeating:Steam and rename DistrictHeating to DistrictHeatingWater #4972

Merged

19 tasks

kbenne approved these changes Sep 19, 2023

View reviewed changes

jmarrec added a commit that referenced this pull request Sep 20, 2023

Make Capacity Fraction Schedule required-field (alwaysOnContinuousSch…

e03655f

…edule) The grand protector of the API has spoken! cf @kbenne 's comment here: #4954 (comment)

jmarrec merged commit 7a3a5ab into v23.2.0-IOFreeze Sep 20, 2023
0 of 4 checks passed

joseph-robertson linked an issue Sep 27, 2023 that may be closed by this pull request

E+ 23.2.0: Wrap DistrictHeating:Steam #4941

Closed

jmarrec deleted the district-heating-steam branch January 15, 2024 09:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Addresses #4941, wrap DistrictHeating:Steam #4954

Addresses #4941, wrap DistrictHeating:Steam #4954

joseph-robertson commented Aug 30, 2023 •

edited

Loading

jmarrec Sep 12, 2023

kbenne Sep 19, 2023

jmarrec Sep 19, 2023

jmarrec Sep 19, 2023

jmarrec Sep 12, 2023

joseph-robertson Sep 12, 2023

jmarrec Sep 12, 2023

kbenne Sep 19, 2023

jmarrec Sep 19, 2023 •

edited

Loading

ci-commercialbuildings commented Sep 12, 2023 •

edited

Loading

kbenne Sep 19, 2023

kbenne Sep 19, 2023

jmarrec commented Sep 19, 2023

Addresses #4941, wrap DistrictHeating:Steam #4954

Addresses #4941, wrap DistrictHeating:Steam #4954

Conversation

joseph-robertson commented Aug 30, 2023 • edited Loading

Pull request overview

Pull Request Author

Review Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmarrec Sep 19, 2023 • edited Loading

Choose a reason for hiding this comment

ci-commercialbuildings commented Sep 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmarrec commented Sep 19, 2023

joseph-robertson commented Aug 30, 2023 •

edited

Loading

jmarrec Sep 19, 2023 •

edited

Loading

ci-commercialbuildings commented Sep 12, 2023 •

edited

Loading