Ideal loads air system ems override enthalpy update correction

[jcyuan2020]

Pull request overview

• Fixes EMS supply temperature overide bug for Ideal Loads (Purchased Air) Air System #8494
• The PR updates the EMS overridden supply air enthalpy timely so that correct value can be used in later parts in the program to evaluate various types of loads.
• Expected diffs: The example file named EMSConstantVolumePurchasedAir.idf is the one with major diffs. The EMS codes in the example file override the supply air temperature, humidity, and mass flowrate. The diffs are in the coil loads, which are demonstrated as follows (where the "Baseline" series are the results from the "develop" branch):

When the total loads are broken down further to sensible loads and latent loads, it is found that the sensible part does not cause the diff:

The diffs in the total load are contributed by the latent loads instead:

The other file that has similar diffs is the python plugin version of EMSConstantVolumnPurchasedAir.idf, named PythonPluginConstantVolumePurchasedAir.idf, which shares a similar EMS block that overrides the supply air temperature, humidity ratio, and mass flow rate.

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

[jcyuan2020]

After some careful consideration, I am proposing to change the code a little bit more, by moving the entire EMS supply temperature and humidity override section

        // EMS override point  Purch air supply temp and humidty ratio ..... but only if unit is on, SupplyMassFlowRate>0.0
        if (PurchAir(PurchAirNum).EMSOverrideSupplyTempOn) {
            PurchAir(PurchAirNum).SupplyTemp = PurchAir(PurchAirNum).EMSValueSupplyTemp;
        }
        if (PurchAir(PurchAirNum).EMSOverrideSupplyHumRatOn) {
            PurchAir(PurchAirNum).SupplyHumRat = PurchAir(PurchAirNum).EMSValueSupplyHumRat;
        }
        SupplyEnthalpy = PsyHFnTdbW(PurchAir(PurchAirNum).SupplyTemp, PurchAir(PurchAirNum).SupplyHumRat);

down a couple more lines inside the protection of the incoming if (SupplyMassFlowRate > 0.0) {.

So the new code looks like this:
14cf1d2#diff-5b31c807ac6b8317860b74f165b2f4ffb48919fc0ff6a8bf27e6d9e6f482e25bL2740-R2749

The rationale for that is a little bit subtle in but lies in the code context and the original comments of this EMS overide should be done only if unit is on, SupplyMassFlowRate>0.0.
From reading the code, if just purely adding the originally proposed change of adding SupplyEnthalpy = PsyHFnTdbW(PurchAir(PurchAirNum).SupplyTemp, PurchAir(PurchAirNum).SupplyHumRat); there is a possibility that the block of EMS override code would also override the SupplyEnthalpy for the zero SupplyMassFlow case, which should have been left untouched from above (spanning quite long distance above):

EnergyPlus/src/EnergyPlus/PurchasedAirManager.cc

Lines 2495 to 2501 in e003a91

	} else { // SupplyMassFlowRate is zero
	SupplyEnthalpy = MixedAirEnthalpy;
	PurchAir(PurchAirNum).SupplyHumRat = PurchAir(PurchAirNum).MixedAirHumRat;
	PurchAir(PurchAirNum).SupplyTemp = PurchAir(PurchAirNum).MixedAirTemp;
	CoolSensOutput = 0.0;
	CoolTotOutput = 0.0;
	}

and here:

EnergyPlus/src/EnergyPlus/PurchasedAirManager.cc

Lines 2731 to 2736 in e003a91

	} else { // SupplyMassFlowRate is zero
	SupplyEnthalpy = MixedAirEnthalpy;
	PurchAir(PurchAirNum).SupplyHumRat = PurchAir(PurchAirNum).MixedAirHumRat;
	PurchAir(PurchAirNum).SupplyTemp = PurchAir(PurchAirNum).MixedAirTemp;
	HeatSensOutput = 0.0;
	}

If the SupplyEnthapy values in the above two (zero SupplyMassFlowRate) locations is overriden by EMS, the impact will be far down here:

EnergyPlus/src/EnergyPlus/PurchasedAirManager.cc

Lines 2867 to 2869 in e003a91

	Node(InNodeNum).Temp = PurchAir(PurchAirNum).SupplyTemp;
	Node(InNodeNum).HumRat = PurchAir(PurchAirNum).SupplyHumRat;
	Node(InNodeNum).Enthalpy = SupplyEnthalpy;

where the expected value would be the MixedAirEnthalpy, but the code will give the enthalpy calculated from using the EMS overridden temperature and humidity ratio, which is definitely not expected in the zero supply airflow case.

Of course, we should definitely still add the original change to include the above line, otherwise the general case of SupplyMassFlowRate>0.0 case could fail to have a timely updated SupplyEnthalpy.

So this proposed additional change would cover both the zero or positive SupplyMassFlowRate cases.
The changes are reflected in
commit 14cf1d2.

[mjwitte]

@jcyuan2020 It shouldn't make any difference in actual results to move that into the massflow>0 block, but I agree it makes sense there.

Looking at this code, in the else { // purchased air OFF block, PurchAir(PurchAirNum).SupplyTemp and PurchAir(PurchAirNum).SupplyHumRat should be set here to match the Node(InNodeNum) values. It's just a reporting thing, but the output variables should match the node conditions for consistency, even when the system is off.

[mjwitte]

Also, it would be good to extend the unit test to cover both the massflow>0 and =0 cases.

[jcyuan2020]

@jcyuan2020 It shouldn't make any difference in actual results to move that into the massflow>0 block, but I agree it makes sense there.

Looking at this code, in the else { // purchased air OFF block, PurchAir(PurchAirNum).SupplyTemp and PurchAir(PurchAirNum).SupplyHumRat should be set here to match the Node(InNodeNum) values. It's just a reporting thing, but the output variables should match the node conditions for consistency, even when the system is off.

@mjwitte Thanks! I added the following lines near the end of the else{// purchased air OFF block like this:

        PurchAir(PurchAirNum).SupplyTemp = Node(InNodeNum).Temp;
        PurchAir(PurchAirNum).SupplyHumRat = Node(InNodeNum).HumRat;

A unit test for zero supply mass flow rate is also added.

[Myoldmopar]

@mjwitte do you anticipate doing a final review with these latest changes? And I don't see an analysis of the diffs (plots, etc.), do you anticipate needing those for wrapping this? Let me know if you want me to try to wrap it up. Thanks!

[mjwitte]

@Myoldmopar I've reviewed the code changes and agree with them. I have not looked at the diffs. @jcyuan2020 Can comment on those.

[jcyuan2020]

@Myoldmopar I've reviewed the code changes and agree with them. I have not looked at the diffs. @jcyuan2020 Can comment on those.

I have updated the topmost comment of this PR. The example file EMSConstantVolumnPurchasedAir.idf is also considered the defect file for the original issue or this PR.

[Myoldmopar]

@jcyuan2020 thank you so much for providing the plots. Those are some big changes on the latent (and total) heat transfer rate. From the code changes I think this is reasonable, but I will need a little time to digest. The lack of impacting other files, and the fact that this has been looked at by a couple folks makes me feel better about it. Does anyone else want to second the changes here while I go off and ponder?

[rraustad]

This all looks reasonable but I am unsure of the difference between setting the supply node condition to the inlet versus the mixed node when the unit is off. @mjwitte ? And when EMS is setting the conditions, regardless if there is flow, shouldn't the supply condition reflect what EMS is writing to the node? I guess at 0 mass flow you can't have a delta T, w or H so this does make sense this way.

[mjwitte]

@rraustad The supplytemp and supplyhumrat struct variables were added recently so that new output variables could be added. Since these are the conditions at the InNode, then they better have the same value. When flow is zero, the InNode conditions are set to match the zone node. That was done long ago, probably to make sure there aren't any funny noise values in the zone air balance calcs.

[rraustad]

I don't see how setting the EMS conditions all the time to only when mass flow > 0 changes the answer. But then there are only 2 changes here so these changes certainly did something. Maybe it was the 2nd change that fixed this and this change just updates based on EMS only when there is flow. I guess that's OK (see other comment).

[jcyuan2020]

@jcyuan2020 thank you so much for providing the plots. Those are some big changes on the latent (and total) heat transfer rate. From the code changes I think this is reasonable, but I will need a little time to digest. The lack of impacting other files, and the fact that this has been looked at by a couple folks makes me feel better about it. Does anyone else want to second the changes here while I go off and ponder?

Thanks for reviewing this! @Myoldmopar @Myoldmopar @rraustad
BTW, I think mysteriously my last few round of changes were missing from the top of the PR--supposedly it should contain 9 graphs (3 for total load, 3 for sensible, and 3 for latent) and more text explains/edits. It looked like GitHub reset that to a state few hours ago when I just posted the first batch--looks like a glitch? I will wait a while to see if it will come back.

[jcyuan2020]

Re-posted the mysteriously missing graphs and texts for the diff analysis.

[Myoldmopar]

Looks like everything is tidied up here nice and CI is pretty happy! I will build and run locally to verify while I am resolving some conflicts in another branch, but I think this will be good to go.

[Myoldmopar]

OK, I had to fix a couple lines in the new unit tests, but after that this ran just fine. I'm going to go ahead and merge this. Thanks @jcyuan2020 and @mjwitte and @rraustad.