Update the function of GetInternalGainDeviceIndex

[LipingWang]

Pull request overview

• Fixes A room air model crashed due to different names between internal gain objects and room air objects for internal gains #8705
• Update the function of GetInternalGainDeviceIndex which checks the consistency between the names of internal gain objects (such as people, light, other equipment) and those referred by the object "RoomAir:Node:AirflowNetwork:InternalGains."

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

[Myoldmopar]

I guess this change works technically, but it seems out of place. With this change, you basically have this:

Loop over all devices
  If device.name matches searchName
    If device.type matches searchType
      ErrorsFound = false, break, and return
    But if device.type doesn't match searchType
      ErrorsFound = true, break, and return
    EndIf
  But if device.name doesn't match searchName    << new line
    ErrorsFound = true, and continue the loop    << new line
  EndIf
EndLoop

So it's weird because you are going to loop over all the devices and you are going to end up setting ErrorsFound to true a good number of times before you find a match. Why go through the trouble of setting ErrorsFound so many times? It should've just been initialized to true before the loop starts and that's all you needed to do. Then it would remain false if the loop terminates without finding a match. And in fact, you could just simplify the logic and remove the line that sets ErrorsFound to false because it would already be initialized to false. It should be reduced to something like:

ErrorsFound = true  << assume we don't find a match
Loop over all devices
  If device.name matches searchName
    If device.type matches searchType
      ErrorsFound = false
    EndIf
    break and return
  EndIf
EndLoop

Seems like a simpler solution, and probably worth making the change before this merges. Or at least let me know why you think we shouldn't do it.

[mjwitte]

@Myoldmopar When I first saw the comment about ErrorFound I was concerned that this may be a case where the argument is passed along to many functions and might already be true when this function is called (as so many other places in the code). But in this case, the argument is localized when the function is called. Seems it would be better for this function to be a bool and remove ErrorFound as an argument, just to keep it clear for future devs, but maybe that's nit-picking. Or maybe it should be an int function and keep the error arg.

[Myoldmopar]

I think it would be reasonable for the function to just return an integer and eliminate the error found bool altogether. Return -1 as an invalid device index. That would eliminate the need to return both an error flag and a valid index flag. And that is something we do quite often in E+ already. (Not that that alone would be precedent...)

One additional note is that if you make a change to initialize the variable to true you may need to add it to the approve list in scripts/dev/find_byref_bool_override.py.

[LipingWang]

@mjwitte @Myoldmopar Thank you for the comments and notes! I will update this as an int function. Should I consider moving the related error message from RoomAirModelManager.cc inside this function?

[mjwitte]

Should I consider moving the related error message from RoomAirModelManager.cc inside this function?

I would leave it where it's at, since it uses some local information to build the error message.

[LipingWang]

@mjwitte @Myoldmopar The function GetInternalGainDeviceIndex was updated. Please let me know for any further suggestions or comments. Thanks a lot!

[amirroth]

If you are returning DeviceIndex, why does it also need to be a reference parameter?

[LipingWang]

@amirroth Thank you for your comments! The returned value for DeviceIndex was to replace ErrorFound in the original function GetInternalGainDeviceIndex. In the same time, DeviceIndex in this function (GetInternalGainDeviceIndex) needs to be passed back to state.dataRoomAirMod->RoomAirflowNetworkZoneInfo(ZoneNum).Node(RAFNNodeNum).IntGainsDeviceIndices(gainsLoop) within the function void GetRoomAirflowNetworkData in RoomAirModelManager.
What would be a better way of handling this? Any recommendations? Thanks a lot!

[amirroth]

Well, if a returned index of -1 implies error then, you can use that to infer the IntGainError flag at the call site. If there is a single error mode that can be specified using a return value (the two most common ones are -1 for IndexNotFound and NULL for ObjectNotFound or MemoryReferenceNotFound) then this is a a good pattern, especially if the error must be handled in the direct caller.

I've noticed EnergyPlus often passes Error parameters through multiple levels of functions. This pattern is less good and has been subsumed by exceptions. I assume that at one point we will do a pass that cleans that up that pattern as well, but probably not in the very near future.

[LipingWang]

@mjwitte @amirroth @Myoldmopar Many thanks for helping me improve the code! The function GetInternalGainDeviceIndex was updated by removing &DeviceIndex as one of the function argument, returning an integer DeviceIndex from the function, and replacing std::string with std::string_view passed by reference. Thank you so much!

[mjwitte]

@LipingWang Please update this branch for @Myoldmopar 's final review.

[Myoldmopar]

Tested this locally with develop pulled in, all good. Thanks!