Dedicated general exhaust system

[jcyuan2020]

Pull request overview

• NFP draft for a new feature development that allows more flexible general exhaust airflows from across multiple airloops.

Currently, the airloop HVAC systems allow zone exhaust to be modeled from the zone exhausts or from return path to OA mixer. This means that the exhaust system will generally be tied to the same airloop system where the supply air is defined.

However, for many modern laboratory or hospital exhaust ventilation designs, there are frequently needs for centralized exhaust system that collect the exhaust air from different zones that could belong to different airloops on the supply side. For such a scenario, currently it requires some cumbersome work-around in order to get an approximate simulation setup in EnergyPlus, if not possible at all.

The NFP proposed a method to enable modeling such general central exhaust system designs. Two new objects---AirLoopHVAC:ExhaustSystem and ZoneHVAC:ExhaustControl will be added to the EnergyPlus idd file. Further, the usage of the current AirLoopHVAC:Mixer object will be expanded to allow proper zone exhausts connections and airflow mixings for the newly added AirLoopHVAC:ExhaustSystem and ZoneHVAC:ExhaustControl objects.

Update:

• Implementation code of the feature now added.

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

[nrel-bot-2b]

@jcyuan2020 @Myoldmopar it has been 28 days since this pull request was last updated.

[nrel-bot-2]

@jcyuan2020 @Myoldmopar it has been 28 days since this pull request was last updated.

[rraustad]

I wasn't going to mention this but if I don't this will just sit here. This "if (allocated...)" doesn't do anything if all calls look like this:

    if (state.dataExhAirSystemMrg->GetInputFlag) { // First time subroutine has been entered
        GetExhaustAirSystemInput(state);
        state.dataExhAirSystemMrg->GetInputFlag = false;
    }

also, the if (allocated) is only true if an array is actually allocated, even if size=0. So, if there are no exhaust systems this will always be false. It would be better for this to look like:

if (!state.dataExhAirSystemMrg->GetInputFlag) return;
state.dataExhAirSystemMrg->GetInputFlag = false;

[jcyuan2020]

Thanks! I think it might be just using the first line:

if (!state.dataExhAirSystemMrg->GetInputFlag) return;
// state.dataExhAirSystemMrg->GetInputFlag = false;

and leave the second line out. Theoretically this will only let the state.dataExhAirSystemMrg->GetInputFlag be set to false after the GetExhaustAirSystemInput(state); is successfully executed (based on the SimExhaustAirSystem() call), rather than just set the input flag false in any conditions.

The original lines were used because of these lines (Lines 125-127):

            if (numExhaustSystems > 0) {
                state.dataZoneEquip->ExhaustAirSystem.allocate(numExhaustSystems);
            }

where numExhaustSystems is equivlently state.dataInputProcessing->inputProcessor->epJSON.find("AirLoopHVAC:ExhaustSystem").value().size(). It seemed that the allocate() only got executed when there are at >0 input blocks in the idf (or json) input file.

[Myoldmopar]

Tiny conflict in the output rules changes document. I'll fix, build and test locally, and push back up for one final round of CI this evening.

[jcyuan2020]

Tiny conflict in the output rules changes document. I'll fix, build and test locally, and push back up for one final round of CI this evening.

Thanks! I just pushed a couple of minor changes and a new merge commit to bring the branch up-to-date.

[Myoldmopar]

In general this looks great. I was wondering about the IDD objects and needing min-fields.

[Myoldmopar]

Is it worth putting min-fields on these new objects?

[jcyuan2020]

Thanks! I think a min-fields 4 would be helpful for the AirLoopHVAC:ExhaustSystem one, which I will add to it. The ZoneHVAC:ExhaustControl might benefit from some number as well but with only min-fields numbers it is not fully defined--there would be multiple different ways to meet that min fields number but then but still be insufficient--so for this one I would like to pass.

[Myoldmopar]

Next time you are creating new classes, just initialize at the variable declaration rather than creating a constructor. No worries for this PR, but just note it for the future.

[jcyuan2020]

Thanks! Will keep in mind. I've pushed in the changes with idd change (min-fields added for AirLoopHVAC:ExhaustSystem).

[Myoldmopar]

I ran the full suite including regressions locally, fully clean. If the IDD objects don't need min-fields, this can merge right in. If it needs min-fields, go ahead and add that and then it can merge.

[mjwitte]

Hmm, ZoneHVAC:ExhaustControl should probably have \min-fields 7 for clarity, but since the input processing is not using GetObjectItem the defaults will be applied, and IDF Editor is ok with this as-is. So, it can stay as-is.

[jcyuan2020]

Thanks! @mjwitte I feel 7 might be a little bit too stringent. Probably a min-fields of 5 or 6?---there are four required fields (so hopefully the # of required fields will implicitly "create" a min-fields of 4), and then an additional (auto-sizable design flow rate) numerical, and an additional choice with default (flow control type, which theoretically could be left blank to take the default). The kind of uncertainty was the reason to leave out this min-fields for this one earlier.

[mjwitte]

Just for the record, in the idf world, my philosophy is to include the most relevant fields to make the object readable. Since Flow Control Type is a key input, I suggested 7 so that automatically generated idf files would include that field. EnergyPlus will accept objects that are shorter than min-fields without error, as long as all required fields are present. And if an object is processed using GetObjectItem min-fields should cover all fields with a default value declared in the IDD (but that's not the case here).