Convective Baseboard Crashing in Sizing Routine When Autosizing is Not Requested

[RKStrand]

Pull request overview

• Fixes Simulation crashes in SizeBaseboard because SetUpZoneSizingArrays was never called #10677
• A user found that for an input file that had a convective baseboard in it that there was a hard crash. This hard crash was traced back to a line in a sizing subroutine that assumed that certain data structures were already allocated. Oddly, this user's file did not request that the baseboard by autosized. It was eventually determined that the MySizeFlag is initialized to true and never reset when autosizing is not requested like in this user input file. With MySizeFlag still set to true, the sizing routines in the convective baseboard unit were called as a result of MySizeFlag being true. Since autosizing was not requested elsewhere in the file either, none of the sizing arrays were allocated, leading to the problem. This fixes this issue by resetting the MySizeFlag to false in the convective baseboard data structure when no autosizing is requested. This avoids then the call to the sizing routine since it does not need to be executed and thus also the referencing of data structures that have not been allocated. The solution tests all three inputs that could be autosized in the convective baseboard unit to see if any autosizing condition is met. If not, MySizeFlag is set to true. This test is done during the reading of the input to avoid it being called more than once. A unit test verifying the correct operation of the new subroutine is included in this pull request.

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

[mjwitte]

@RKStrand This fixes the defect file, but it still doesn't protect against trying to size without sizing active.
e.g. Take the user file and change the heating capacity to "autosize" - vector crash.
Take the user file and leave the baseboard hardsized, but change the fan to autosize:

   ** Severe  ** For autosizing of Fan:SystemModel UNIT1_ROOM_AC_SUPPLY_FAN, a system sizing run must be done.
   **   ~~~   ** The "SimulationControl" object did not have the field "Do System Sizing Calculation" set to Yes.
   **  Fatal  ** Program terminates due to previously shown condition(s).

So something is still missing in the baseboard sizing to check if sizing is active. @rraustad probably knows this by heart.

[rraustad]

From the defect description, the FinalZoneSizing array won't exist if there is no sizing run:

auto const &finalZoneSizing = state.dataSize->FinalZoneSizing(state.dataSize->CurZoneEqNum);

because those arrays are allocated in this call:

if (state.dataGlobal->ZoneSizingCalc) {
    SizeZoneEquipment(state);

That line should be moved down to after this check, for the places it's needed, and this check should be in all the correct places because it may be that only 1 input is autosized. If the flag this->MySizeFlag does actually work correctly then this check could be the first thing that happens (because you know that this zone equipment needs to be autosized) and only have to do it once, but that's risky if that flag is not set correctly.

CheckZoneSizing(state, cCMO_BBRadiator_Water, this->EquipID);

[Myoldmopar]

OK, so this still needs some work, I'm going to just move on from this for now.

[RKStrand]

Thanks for the feedback, @mjwitte and @rraustad. I will look into this as soon as possible and will let you know when I have hopefully resolved this.

[RKStrand]

I believe that the flag will now be set correctly based on the user input for this object. However, as @mjwitte pointed out, the file will crash in its current form if you set any of the three key input parameters to autosize. The file (I think this is a contradiction when you set one of those inputs to autosize without doing other things) says "NO" to zone sizing. If you want a piece of zone equipment sized, shouldn't you have to actually tell E+ that you are sizing it? I think that is what @rraustad is pointing out. The question now is what to do about it.

Here is what I propose: in the new subroutine which checks for the presence of things that are autosized, also check to see if the user has requested zone sizing (state.dataGlobal->DoZoneSizing = true), then everything should be fine and other existing checks will weed out any other problems. However, if DoZoneSizing is false but one of the baseboard parameters is autosize, send a severe/fatal stating that in order to autosize a baseboard unit that flag in the SimulationControl input must be set to "Yes" and other inputs need to be set properly.

Would this address the concern here, assuming that the correct checks are being made (I think they are) for autosizing baseboard inputs?

[rraustad]

@RKStrand CheckZoneSizing is the call that does the check you suggest and SizeBaseboard is already set up to do that. It's just that the FinalZoneSizing shortcut is set before that check. I guess it makes no difference if CheckZoneSizing is called from GetBaseboardInput or SizeBaseboard (like all other models in E+). Your choices are to move the shortcut for FinalZoneSizing below the call to CheckZoneSizing, or call CheckZoneSizing from the new function in GetBaseboardInput and delete all the calls to CheckZoneSizing in SizeBaseboard.

[RKStrand]

I have just made a commit that should fix the issue that @mjwitte noted using one of the methods that @rraustad recommended. Hopefully, this addresses the concerns and still comes back green. I went with doing the CheckZoneSizing from the new subroutine and getting rid of all of the other calls. That seemed to me to be more "efficient" and clearer though different from some other models.

[Myoldmopar]

@RKStrand I'm not sure if you saw or not, but this has got 3 failing unit tests that would need to be addressed:

• 352 - integration.HVACStandAloneERV_Economizer (Failed)
• 664 - integration.VAVSingleDuctReheatBaseboard (Failed)
• 696 - integration.WindACRHControl (Failed)

[RKStrand]

@Myoldmopar Yes, I did see that. I will try to address that either today or tomorrow. I know why they are failing and should be able to edit those files so that they don't crash. In my opinion, they should not have worked previously because they autosize without actually requesting an autosizing run.

[github-actions]

⚠️ Regressions detected on macos-14 for commit b70ed78

Regression Summary

• Audit: 3
• EIO: 3
• ERR: 3
• MDD: 3
• MTD: 3
• RDD: 3
• Table Big Diffs: 3
• Table String Diffs: 3

• View Results
• Download Regressions

[Myoldmopar]

Looks like maybe @mjwitte's concerns were addressed and CI is now happy. If you agree @mjwitte, please go ahead and mark your review as approved and we'll get this on the path to merge. Thanks!

[mjwitte]

This is all good now. Thanks @RKStrand
Merge-queued.

[Myoldmopar]

And guess what it's all clean. Thanks @RKStrand and @mjwitte !