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

LowTempRadiantSystem - Fix minor bug #8463

Merged
merged 5 commits into from
Feb 12, 2021
Merged

LowTempRadiantSystem - Fix minor bug #8463

merged 5 commits into from
Feb 12, 2021

Conversation

jmythms
Copy link
Contributor

@jmythms jmythms commented Jan 11, 2021 •
edited
Loading

Pull request overview

NOTE: ENHANCEMENTS MUST FOLLOW A SUBMISSION PROCESS INCLUDING A FEATURE PROPOSAL AND DESIGN DOCUMENT PRIOR TO SUBMITTING CODE

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, the author must demonstrate they are justified using plots and descriptions
  • If changes fix a defect, the fix should be demonstrated in 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
  • If structural output changes, add to output rules file and add OutputChange label
  • If adding/removing any LaTeX docs or figures, update that document's CMakeLists file dependencies

@jmythms jmythms added the Defect Includes code to repair a defect in EnergyPlus label Jan 11, 2021
@jmythms jmythms self-assigned this Jan 11, 2021
@jmythms jmythms changed the title Fix minor bug LowTempRadiantSystem - Fix minor bug Jan 11, 2021
Myoldmopar
Myoldmopar reviewed Jan 12, 2021
View reviewed changes
src/EnergyPlus/LowTempRadiantSystem.cc
@@ -698,7 +698,7 @@ namespace LowTempRadiantSystem {
}
} else if (UtilityRoutines::SameString(Alphas(12), "CapacityPerFloorArea")) {
thisRadSys.CoolingCapMethod = CapacityPerFloorArea;
if (!lNumericBlanks(9)) {
if (!lNumericBlanks(11)) {
Copy link
Member

Choose a reason for hiding this comment

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

Interesting that this wasn't causing any issues. And interesting that no unit test caught this. @RKStrand could you give a quick thumbs up/down on this?

@jmythms it would be really great to integrate testing of this in a unit test. Is there an existing unit test that handles input processing of this that you could modify or mimic? (Hint: there must be one because several lines of code around this line you changed are indeed covered by unit test.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the review! Working on the unit test now.

While working on this, I also just came across this line, which I am not sure should have been

if (thisRadSys.ScaledCoolingCapacity <= 0.0) {
instead of
if (thisRadSys.CoolingCapMethod <= 0.0) {.

I think this second change would generate the error message intended:
image

for this input object:
image

Copy link
Contributor

Choose a reason for hiding this comment

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

@Myoldmopar I would agree that this is a bug that needs to be fixed. It should be changed as @jmythms is suggesting.

My guess (and it's just a guess because I don't think I was the one to add all of the sizing mods that did things on a floor area, etc. basis) is that there simply isn't a test that exercises all of the potential options for both correct and incorrect input for low temperature radiant systems. Is there one for a different input object like baseboards or something similar, maybe and if so, it would be good to implement such a unit testing scheme here.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jmythms Yes, I agree with your second recommended change also. It should not be looking at CoolingCapMethod but ScaledCoolingCapacity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you!

@Myoldmopar
Copy link
Member

@jmythms let me know if you need anything on this. It's just a bug fix, so there's no urgency to get it in, but I am trying to get the PR list down and this would be a quick fix. I already pulled in develop and ran tests locally.

@jmythms jmythms marked this pull request as draft February 8, 2021 17:09
@jmythms
Copy link
Contributor Author

jmythms commented Feb 8, 2021

@jmythms let me know if you need anything on this. It's just a bug fix, so there's no urgency to get it in, but I am trying to get the PR list down and this would be a quick fix. I already pulled in develop and ran tests locally.

Sorry, I got sidetracked with the other projects, will get this ready quick.

@jmythms jmythms marked this pull request as ready for review February 9, 2021 07:38
@jmythms jmythms added this to the EnergyPlus 9.5.0 IOFreeze milestone Feb 9, 2021
@jmythms
Copy link
Contributor Author

jmythms commented Feb 9, 2021

@jmythms let me know if you need anything on this. It's just a bug fix, so there's no urgency to get it in, but I am trying to get the PR list down and this would be a quick fix. I already pulled in develop and ran tests locally.

@Myoldmopar This is ready for review, can you take a look please?

@Myoldmopar
Copy link
Member

This looks ready to go. Thanks for adding the unit test. I'm going to do a quick build of this with develop pulled in just to make sure nothing happened with the unit test. Then I'll merge. Thanks @jmythms

@jmythms
Copy link
Contributor Author

jmythms commented Feb 12, 2021

This looks ready to go. Thanks for adding the unit test. I'm going to do a quick build of this with develop pulled in just to make sure nothing happened with the unit test. Then I'll merge. Thanks @jmythms

Thanks very much @Myoldmopar

@Myoldmopar
Copy link
Member

Built with develop pulled in locally, no issues. Ran unit tests serially then the whole test suite in parallel, all passing. Merging.

@Myoldmopar Myoldmopar merged commit 0c192f2 into develop Feb 12, 2021
@Myoldmopar Myoldmopar deleted the LowTempRadiantCheckBugFix branch February 12, 2021 18:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RKStrand RKStrand RKStrand left review comments

@Myoldmopar Myoldmopar Myoldmopar left review comments

Assignees

@jmythms jmythms

Labels
Defect Includes code to repair a defect in EnergyPlus
Projects
None yet
Milestone
EnergyPlus 9.5.0 IOFreeze
Development

Successfully merging this pull request may close these issues.

Check should point to 'Cooling Design Capacity Per Floor Area' instead of 'Heating Control Throttling Range'.
8 participants
@jmythms @Myoldmopar @RKStrand @nrel-bot @nrel-bot-2 @nrel-bot-3 @nrel-bot-2b @nrel-bot-2c