Correct Use of Incorrect Index for VRF Cooling

[RKStrand]

Pull request overview

• Fixes VRF sizing is looking at wrong type of sizing, e.g., looking at heating when it should be looking at cooling #10638
• In the VRF code for sizing options (capacity, per floor area, etc.), the code had a problem for cooling in that it was looking at the variable for heating incorrectly. This was corrected to look at the correct flag now. In addition, the code to interpret the heating and cooling parameters which had a lot of similarities was turned into a subroutine to reduce the number of lines of code as well as duplication. In addition to a unit test of this new subroutine that corrects the issue, two types in input files were corrected and an existing IDF in the test suite was enhanced so that this section of code will get used in at least one file. No differences in output were encountered except in the enhanced IDF since this results in a change in how the sizing was done.

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, 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

Reviewer

This will not be exhaustively relevant to every PR.

• Perform a Code Review on GitHub
• If branch is behind develop, merge develop and build locally to check for side effects of the merge
• 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
• Check that performance is not impacted (CI Linux results include performance check)
• Run Unit Test(s) locally
• Check any new function arguments for performance impacts
• Verify IDF naming conventions and styles, memos and notes and defaults
• If new idf included, locally check the err file and other outputs

[rraustad]

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?

initSizingVars(state, sizingMethod, HVACSizingIndex) 

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?)

sizingCoolingAirFlow.size(state, TempSize, errorsFound)

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:

void BaseSizerWithScalableInputs::setHVACSizingIndexData(int const index)
{
    this->zoneHVACSizingIndex = index;
}

[RKStrand]

@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?

[rraustad]

@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.

[Myoldmopar]

That has got a conflict now, but it must not be too intense. I can give resolving it a try if this is worth getting into 24.2. I'm hoping to start RCs later this week, so I'm just wanting to identify which things to work on and which things to skip.

[RKStrand]

@Myoldmopar Well, I'd like to get it done for V24.2 since it's part of my deliverables and it would be nice to get that all taken care of to end the FY cleanly. I'll try to take a quick look at the conflict and figure out what the issue is. Hopefully it's not a big deal.

[RKStrand]

@Myoldmopar I'm not sure why this created a conflict except that maybe my changes were "staged" in multiple steps. Basically, my code in this branch replaces the develop code. All of the "old" develop code that it is showing as a "conflict" was removed here and moved to the new subroutine that is now being called. It's kinda of a non-issue in my opinion but maybe it got confused because other changes were made in this file. I'm not sure how I resolve it locally and I am not sure what steps are involved in resolving the conflict via a browser. In any case, this is very easy to fix--just get rid of the develop lines that were flagged and leave the new code intact. That should resolve the conflict and implement the fix.

[RKStrand]

So, yes, if we can get this resolved and merged for V24.2 that would be helpful and should take very little effort.

[RKStrand]

@Myoldmopar Ok, I didn't realize that I could do this directly on the GitHub site. Apparently, I can! So, I went ahead and resolved this. Hopefully, things still come back green and this will be ready to merge.

[github-actions]

⚠️ Regressions detected on macos-14 for commit fee19b3

Regression Summary

• EIO: 1
• ERR: 1
• ESO Big Diffs: 1
• MTR Small Diffs: 1
• Table Big Diffs: 1
• Table String Diffs: 1

• View Results
• Download Regressions

[github-actions]

⚠️ Regressions detected on macos-14 for commit 079c88c

Regression Summary

• EIO: 1
• ERR: 1
• ESO Big Diffs: 1
• MTR Small Diffs: 1
• Table Big Diffs: 1
• Table String Diffs: 1

• View Results
• Download Regressions

[Myoldmopar]

Diffs as expected, and otherwise clean. Merging this. Thanks @RKStrand