PV Zone Multiplier Fix #6283
PV Zone Multiplier Fix #6283
Conversation
PV systems were not picking up the zone multiplier. This now fixes the problem. The defect input file that uses the zone multiplier is now in line with the file that has all of the zones defined. Unit tests for the fix are also included.
|
@Myoldmopar @lgentile Here is the pull request for this. Results between the two input files noted in the GitHub Issue (#6222) now should identical results in the electric load center output. I know this was a priority issue for OpenStudio, so I'm over notifying that this is ready to go to make sure it is not lost in all of the other things going on in the code right now. I suggested Edwin as a reviewer, but that is just a suggestion. Thanks! |
|
Thank you @RKStrand ! This will help us move forward with our other OS-based project. |
src/EnergyPlus/Photovoltaics.cc
Outdated
|
|
||
| // SUBROUTINE LOCAL VARIABLE DECLARATIONS: | ||
| int thisZone; // working index for zones | ||
| int thisSurface; // index for PV surface | ||
| Array1D< bool > PVarrayZoneInitialized; // avoid doing the zone check too often |
There was a problem hiding this comment.
Does this really persist through multiple calls into this function? It looks like it's just a non-static local variable, so it will be cleaned back up each time this function is called. And the FirstTimeThru variable is similar, it will just be true each time the function gets called again, right?
Here's what I'd like to propose. Instead of adding your own new array here and first time flag, just add a single bool to the main PV array called bool zoneInitialized;. Then you can let it do the allocations, you don't need an allocated flag, you don't need static variables, and you will actually have fewer code changes to accomplish this same task.
If we kept it the way it currently is, you'd need to make the local variables static first, and then because they are static you'd need to clean them up in the clear_state function of this module. (And if it doesn't have a clear_state you'd need to write one of your own and add it to the EnergyPlusFixture unit test class....) In summary, avoid statics when possible! 😃
Let me know if you have any issues with this. The actual code fix here is great and I appreciate you getting it done so quickly.
There was a problem hiding this comment.
@Myoldmopar I don't think that should be a problem. I will work on that as soon as I can. I was thinking keeping it local would be better, but if what you have proposed is better, I'm all for that. Thanks for the review and suggestion. And glad that I could help on this!
src/EnergyPlus/Photovoltaics.cc
Outdated
|
|
||
| if ( ! PVarrayZoneInitialized( PVnum ) ) { | ||
| if ( Surface( thisSurface ).Zone == 0 ) { // might need to get the zone number from the name | ||
| Surface( thisSurface ).Zone = FindItemInList( Surface( thisSurface ).ZoneName, Zone, NumOfZones ); |
There was a problem hiding this comment.
Why isn't this done in GetInput?
There was a problem hiding this comment.
@rraustad Good point. I will change this so that it gets the Zone index directly during GetInput and store it in the local PV data structure. That should then address what you and Edwin pointed out and still fix the issue. Thanks!
This commit addresses the review comments. No more static variables and the zone pointer is gotten during the Get phase in a new function call. Unit tests were also updated to reflect the revised code.
|
@Myoldmopar @rraustad Ok--I have changed the code to reflect both of your comments. The zone number is now kept in the PV array data structure and it is filled during the GetInput phase rather than during report. The whole thing is a lot cleaner now--thanks for your comments! Please let me know if there are any other concerns. If there aren't any, feel free to accept, merge, etc. at will. |
|
Thanks @RKStrand, I'm pulling and building locally real quick to verify and then I'll merge it in. |
Pull request overview
This work addresses GitHub Issue #6222 which showed that the zone multiplier was not being applied to the PV arrays. This fixes the problem so that the zone multiplier is applied properly when PV surfaces are part of a zone.
Work Checklist
Add to this list or remove from it as applicable. This is a simple templated set of guidelines.
Review Checklist
This will not be exhaustively relevant to every PR.