Initialize NodeData pressure in class initializer and DefaultNodeValues

[tanaya-mankad]

Pull request overview

• Fixes NodeData Pressure is not initialized with physical value #10484
• Improve default-constructed state of air nodes

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
• 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

This code change sets the default initialized value of NodeData pressure to 1 atm = 101325.0 Pa. The associated DefaultNodeValues pressure value is also matched so that checks against the default throughout the code will work.
A collateral bug was discovered in the Fan Energy Index calculation, which uses default NodeData values before nodes are fully specified. The calculation was modified to use StdRhoAir instead of calculating it at unphysical node conditions. This modification causes 60 files to display regression errors (only in the FEI).
One additional regression error was caused by a discrepancy between in-class initialized MassFlowRateMax (-999) and DefaultNodeValues default for MassFlowRateMax (0.0). It goes back to Autodesk days and we believe this PR address it correctly.

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

[rraustad]

I may have missed this last time I looked at these code changes. This change to StdRhoAir is causing the diffs. The reorganizing of unit test node initialization, I think, doesn't do anything since the getInput functions don't do any node data manipulation (that's why I was surprised there were diffs). So I think the changes to unit test aren't necessary. The question is what does the FEI Standard say about fan inlet air density. I found this here which shows use of both actual and reference air density. In the function report_fei is inletRhoAir / state.dataEnvrn->StdRhoAir. So you can't change this. But you can make sure that inletAirTemp and InletAirHumrat make sense as to a reasonable, valid, inlet air condition. What is used now for inletAirTemp and InletAirHumrat in this call?

[tanaya-mankad]

In the rho_air calculation as it stood, all of the input conditions were the conditions as set at the inlet node of the fan (data members of the class set during sizing). Problem is, the node hasn't been set up when this line is calculated! It's holding its default as-constructed values, which means InletAirTemp = InletAitHumRat = Inlet pressure = 0.0.

I imagined that using StdRhoAir was appropriate only because that's how it's done in this line of Fans.cc. However, from your discovery, it appears we have a real bug in both the calculation, and where it's done in the code.

To the other point, one might hope that unit tests would be monolithic. But this CI result says otherwise. Here, it's because SetOANodeValues is called from GetVRFInput, and SetOANodeValues can only depend on the global environment. This kind of unintentional coupling is irrationally hard to expect - but also ubiquitous!

[Myoldmopar]

However, from your discovery, it appears we have a real bug in both the calculation, and where it's done in the code.

Could you clarify if this means more changes are coming for this PR?

This kind of unintentional coupling is irrationally hard to expect - but also ubiquitous!

Yes. But also, you should have seen the state of unit testing EnergyPlus when we first added it 10 years ago :) :)

[tanaya-mankad]

I think I'd like some input from @nealkruis and @rraustad , given that a) the FEI calculation uses StdRhoAir as an input elsewhere and b) FEI for the node was clearly dead wrong before. My naïve vote is to leave it, and start an Issue about this using Rich's comment as a starting point.

[nealkruis]

The larger problem with FEI is that it is not a single rating metric. i.e., there are not a set of standard conditions it should be calculated at. Instead it can be calculated at any set of operating conditions (called a "duty point"). EnergyPlus is calculating FEI as as a single metric at design flow and, as such, it doesn't seem appropriate to use a different air density. If we were calculating FEI on an hourly basis we would want to use the actual inlet air density, but as a single data point, I think we should either use standard air density or air density based on the elevation of the site.

[tanaya-mankad]

@rraustad Thoughts? It appears that, as a single value, the FEI calculated here will be / has been incorrect in one of multiple possible ways. How can we move forward so this PR isn't responsible for an entirely new FEI model?

[rraustad]

My recommendation is to revert the change to the FEI calculation and open a new issue specifically for that. The ANSI Standard FEI calculation is using a ratio of moist air density to standard air density = 1.2, which is dry air density at sea level at 21 C. StdRhoAir is used as the denominator in report_fei and is dry air density at elevation. So the FEI equation is using the wrong denominator.

state.dataEnvrn->StdBaroPress = DataEnvironment::StdPressureSeaLevel * std::pow(1.0 - 2.25577e-05 * state.dataEnvrn->Elevation, 5.2559);
state.dataEnvrn->StdRhoAir = Psychrometrics::PsyRhoAirFnPbTdbW(
    state, state.dataEnvrn->StdBaroPress, DataPrecisionGlobals::constant_twenty, DataPrecisionGlobals::constant_zero);

This would need to be discussed by the team. but we could fix the denominator in report_fei and then pass in StdRhoAir (StdRhoAir/StdRhoAirAtSeaLevel) or choose a moist air density at elevation, like 24 C dry-bulb and 50% RH, and document and use that ratio (MoistRhoAir/StdRhoAirAtSeaLevel) for the calculation of FEI. Then just document the changes.

[rraustad]

I don't have a problem with this change. A default is a default. @mjwitte ?

[mjwitte]

1. Yes, this is fine, but it should be DataEnvironment::StdPressureSeaLevel; instead of a hard value.
2. Do we still need the constructors for this below?
   

   EnergyPlus/src/EnergyPlus/DataLoopNode.hh

   Lines 491 to 497 in 1ae0861

   	// Default Constructor
   	NodeData() = default;
   	// Member Constructor
   	NodeData(NodeFluidType const FluidType, // must be one of the valid parameters
   	int const FluidIndex, // For Fluid Properties
   	Real64 const Temp, // {C}

3. What I've run into in unit tests is this being zero, because it gets used heavily in Psychrometric calls.
   

   EnergyPlus/src/EnergyPlus/DataEnvironment.hh

   Line 116 in 4d39dff

   Real64 OutBaroPress = 0.0; // Current outdoor air barometric pressure

   
   It would be great to initialize this to DataEnvironment::StdPressureSeaLevel if that doesn't cause any hiccups.

[tanaya-mankad]

First two comments addressed!
Because this PR is in response to a specific bug and only deals with NodeData, I'm going to hold off on #3 - just getting to here with single-line changes exposing multiple test and regression failures, I am cagey about changing anything outside of a very tiny scope. Happy to create another issue for it though.

[rraustad]

These DataLoopNode::SensedNodeFlagValue initializations here mean something to set point managers and model node checking. Search for SensedNodeFlagValue. Just make sure these are all correct by comparing the initialization here to what is shown in the NodeData struct. I see 6 SensedNodeFlagValue here and 7 in NodeData, So you may have fixed something with this change.

[tanaya-mankad]

This is what's mentioned in the PR comment, though I got lazy and said -999 instead of SensedNodeFlagValue because, typing.

[rraustad]

There is definitely a big change in results for FEI. I expected the result to be lower now, closer to 1. Instead the answer diverged to 3. Numbers I have seen elsewhere are between 0.5 and 1.5. Since there are only 8 diff files, that makes me think that the call to report_fei is too early in some cases. Not sure why but it seems like this report could be done later, like when output reports are actually written. That is, of course, for another branch. I say merge this branch and directly follow up with the fix for FEI calculations.

[rraustad]

@tanaya-mankad can you do a quick branch using StdRhoAir/StdRhoAirAtSeaLevel to see what the FEI looks like? by correcting this equation with sea level air density as the denominator and passing in StdRhoAir.

assert(state.dataEnvrn->StdRhoAir > 0.0);
// Calculate reference fan shaft power
Real64 _refFanShaftPower = (_designFlowRate + 0.118) * (_designDeltaPress + 100 * _inletRhoAir / state.dataEnvrn->StdRhoAir) / (1000 * 0.66);

[rraustad]

With a quick change to FEI calcs I get similar results to this branch. Now I'm not sure if it's an input issue or I did something wrong.

    designPointFEI = FanSystem::report_fei(state, _volFlow, _ratedPower, deltaPress, state.dataEnvrn->StdRhoAir);

Real64 FanSystem::report_fei(
  EnergyPlusData &state, Real64 const _designFlowRate, Real64 const _designElecPower, Real64 const _designDeltaPress, 
Real64 _inletRhoAir)
{

// Calculate reference fan shaft power
Real64 rhoAirSeaLevel = Psychrometrics::PsyRhoAirFnPbTdbW(
    state, DataEnvironment::StdPressureSeaLevel, DataPrecisionGlobals::constant_twenty, DataPrecisionGlobals::constant_zero);
Real64 _refFanShaftPower = (_designFlowRate + 0.118) * (_designDeltaPress + 100 * _inletRhoAir / rhoAirSeaLevel) / (1000 * 0.66);

[nealkruis]

@rraustad, let's make both report_fei calls consistent (using StdRhoAir). Can you make a new issue to investigate FEI calculations, so we can come back to it later?

[rraustad]

@nealkruis if the report_fei call arguments are changed here there will be diffs not associated with node initializations. I would prefer to make any changes to the FEI model in a separate branch. I suspect that the FEI values now (those FEIs = 3), with this change, are more correct now than before. And when the FEI calculations are changed I would rather see the diffs wrt these changes as the baseline (i.e., I don't know yet if the call will use StdRhoAir or some other moist air density). I also assume the FEI changes will follow up this branch before the next release.

[tanaya-mankad]

@rraustad We're agreed that we'll keep changes to the FEI model in a new future branch, and try to merge these (8) regression diffs with this branch in its current state. I'll request a re-review just for completeness.