-
Notifications
You must be signed in to change notification settings - Fork 398
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
Correct Use of Incorrect Index for VRF Cooling #10679
Merged
Merged
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
56e6b6c
10638 Recommended Fix
RKStrand 195aee7
10638 Reorganization and Input File Mods
RKStrand ffc0ff6
Merge branch 'develop' into 10638VRFSizingBasedOnWrongSizingType
RKStrand 0be1cd6
10638 Unit Test
RKStrand 9604332
Merge branch 'develop' into 10638VRFSizingBasedOnWrongSizingType
RKStrand 7da2b23
10638 Clang format check
RKStrand File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will certainly avoid the cut-n-paste errors that caused this issue. Is there a chance that the other 12 parent objects will get this change (I searched for FractionOfAutosizedCoolingCapacity)? Or at the very least a new issue opened?
The remaining discussion is for future use.
Now to think outside the box even further... What is really needed is for this "scaling" code to be moved to the sizer class so it's in 1 place. So that each parent does not need this overhead. All the sizer code would need to know is state.dataHVACVarRefFlow->VRFTU(VRFTUNum).HVACSizingIndex. That is, of course, for another day. That said, maybe the arguments could be shortened to pass only the HVACSizingIndex? and a key?
where sizingMethod = CoolingAirFlowSizing, HeatingAirFlowSizing, CoolingCapacitySizing, or HeatingCapacitySizing (did I miss anything?). Maybe that's too much to ask for this issue but it's good to be thinking in that direction (for the sake of moving this to the sizing routines). In this future code I still want to be able to call the sizing like this (maybe with an HVACSizingIndex argument?)
but let the sizer handle the scaling. If I were calling CoolingAirFlowSizing I could look up the "cooling air flow" methods on the HVACSizing object because I already know I am sizing cooling air flow. Or set up all scalable sizing in the base class and each sizer uses what is relevant. This was considered when the sizer method was first developed but I decided to punt. Here's a hook, and then see the bottom of initializeWithinEP where this was initially started and then abandoned:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rraustad I like your line of thinking here. It would be really nice if this could be standardized across the various models that do this and your thoughts and expertise here are clearly very valuable.
I took a quick look at the various other occurrences in the code using a search for FractionOfAutosizedCoolingCapacity like you did. It seems like there are maybe a couple of cases where the code at first glance at least looks fairly similar to what existed here before the fix was implemented. However, there are other cases where additional steps are being taken. The question is then: how do we accommodate that and still get something that avoids the duplication of code across numerous instances. I really would like to see that happen, but I'm also not sure how that would work at this time. I wonder if that is why you "punted" as you said in your comment?
From my perspective, I am glad that my "value add" to the code here got this conversation started (or restarted?) and might yield other improvements to the code. It would have been easy to just change the one line and move on, but I thought it made more sense to also come up with something that avoids the problem. However, given my relatively low level of experience with the VRF and sizing code, I'm not sure that I'd be the most efficient person to handle anything beyond this. So, I think my vote would be to not attempt to bump the complexity of this up and do more changes here but to definitely open up a new issue that includes your comments here.
Thoughts? Any of the other flagged reviewers want to offer their opinions here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RKStrand I'm fine with leaving this as-is. I punted because the sizing functions were enough work on their own without adding in the rewriting and associated testing of scalable sizing in the zone equipment. I did leave the hook in as a starting place for this "sizable" refactor.