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.

Sign up for GitHub

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

Jump to bottom

Support undisturbed ground temperature models on GroundHeatExchangerVertical #4932

Merged
merged 21 commits into from
Aug 1, 2023
Merged

Support undisturbed ground temperature models on GroundHeatExchangerVertical #4932

merged 21 commits into from
Aug 1, 2023

Conversation

joseph-robertson
Copy link
Collaborator

@joseph-robertson joseph-robertson commented Jul 17, 2023 •
edited
Loading

Pull request overview

Fixes #4930.

Checklist:

  • Add "Undisturbed Ground Temperature Model" to idd
  • Update model files
  • Update model tests
  • Update FT
  • Update FT tests
  • Update VT
  • Update VT tests

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: Support undisturbed ground temperature models on GroundHeatExchangerVertical OpenStudio-resources#199
  • 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 - Model Pull Request - Ready for CI This pull request if finalized and is ready for continuous integration verification prior to merge. IDDChange labels Jul 17, 2023
@joseph-robertson joseph-robertson self-assigned this Jul 17, 2023
@joseph-robertson joseph-robertson mentioned this pull request Jul 18, 2023
Merged
23 tasks
@joseph-robertson joseph-robertson marked this pull request as ready for review July 27, 2023 17:53
kbenne
kbenne reviewed Aug 1, 2023
View reviewed changes
src/energyplus/ForwardTranslator/ForwardTranslateGroundHeatExchangerVertical.cpp

if ((value = modelObject.groundThermalHeatCapacity())) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens to these fields if they are set on the GroundHeatExchangerVertical system? If looks like they might be ignored. Maybe the implementation of these methods like GroundHeatExchangerVertical::groundThermalHeatCapacity() should map to the contained ground temperature model?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ghxVert.groundThermalConductivity() and ghxVert.groundThermalHeatCapacity() both still get translated above.

ghxVert.groundTemperature() previously was only translated to the groundTempModel. AverageSoilSurfaceTemperature for the kusuda object is now handled by its own averageSoilSurfaceTemperature() method.

This is basically the same approach we used for the HorizontalTrench implementation - the ghxVert gets the soilThermalConductivity() and soilSpecificHeat, and then kusuda FTs its own averageSoilSurfaceTemperature().

Copy link
Contributor

Choose a reason for hiding this comment

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

sorry missed the above. Got it.

src/energyplus/Test/GroundHeatExchangerVertical_GTest.cpp
EXPECT_TRUE(ghx.setBoreHoleLength(80.0));
EXPECT_TRUE(ghx.setBoreHoleRadius(0.7E-01));
EXPECT_TRUE(ghx.setGroundThermalConductivity(0.7));
EXPECT_TRUE(ghx.setGroundThermalHeatCapacity(0.3E+07));
Copy link
Contributor

Choose a reason for hiding this comment

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

Like I mentioned. Do these methods do anything now?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These 2 still translate to the ghxVert object (see explanation above).

src/model/GroundHeatExchangerVertical.hpp Show resolved Hide resolved
src/model/GroundHeatExchangerVertical.hpp Show resolved Hide resolved
@ci-commercialbuildings
Copy link
Collaborator

ci-commercialbuildings commented Aug 1, 2023 •
edited
Loading

CI Results for c10e1b7:

joseph-robertson
joseph-robertson commented Aug 1, 2023
View reviewed changes
developer/doc/ReleaseNotes/OpenStudio_Release_Notes_3_7_0_TDB.md
@@ -78,6 +78,9 @@ You can also refer to the [OpenStudio SDK Python Binding Version Compatibility M
* Fix #4695 - AirLoopHVACUnitarySystem: Supply Air Flow Rate Method During <XXX> Operation should be set via related setters/autosize
* Breaks the return of `supplyAirFlowRateMethodDuringCoolingOperation` and `supplyAirFlowRateMethodDuringHeatingOperation`: now returns `std::string` instead of `boost::optional<std::string>`
* Deprecates many set/reset methods
* [#4932](https://github.com/NREL/OpenStudio/pull/4932) - Support undisturbed ground temperature models on GroundHeatExchangerVertical
* Fix #4930 - Support undisturbed ground temperature models on GroundHeatExchangerVertical
* Update `GroundHeatExchanger:Vertical` to actually use the Ground Temeprature Model field
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Reminder to fix spelling of "Temeprature". (This was copied from the 3.6.0 release notes where it is misspelled.)

@joseph-robertson joseph-robertson merged commit 34180f2 into develop Aug 1, 2023
2 checks passed
@joseph-robertson joseph-robertson deleted the ghx-vert branch August 1, 2023 22:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kbenne kbenne kbenne approved these changes

Assignees

@joseph-robertson joseph-robertson

Labels
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support undisturbed ground temperature models on GroundHeatExchangerVertical
3 participants
@joseph-robertson @ci-commercialbuildings @kbenne