Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Space Sizing and HVAC Part 4 #10566

Merged
merged 45 commits into from
Aug 19, 2024
Merged

Space Sizing and HVAC Part 4 #10566

merged 45 commits into from
Aug 19, 2024

Conversation

mjwitte
Copy link
Contributor

@mjwitte mjwitte commented Jun 17, 2024

Pull request overview

ToDoList for post 24.2-I/O Freeze

  • Look for other ToDo items from this and prior PRs
  • Refactor zone equip config return node arrays into a struct

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 mjwitte added NewFeature Includes code to add a new feature to EnergyPlus IDDChange Code changes impact the IDD file (cannot be merged after IO freeze) labels Jun 17, 2024
@amirroth
Copy link
Collaborator

Yassssss king! 👑


* Refine existing Space-level HVAC simulation.

* Extend space HVAC to support more special objects (e.g. ZoneThermalChimney, if budget allows).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A reasonable improvement for spaces. Shouldn't the new field Type of Space Sum to Use include an option to revert back to zone sizing (e.g., None)? and be the default? I guess I'm not clear on what trigger allows spaces, instead of zones, to size zone equipment, and what zone equipment is targeted here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My thought was that the coincident option would use the current zone (lumped spaces) result.

I should highlight here that the same thermostat setting is applied to the zone and its spaces. So, it's not possible in this plan to have a space within a zone set to a different temperature, for example.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, and hopefully sum of space coincident sizing adds up to what zone sizing shows within some small tolerance. I don't see why it wouldn't but you never know (space wall loads?, diffs in window solar gains? etc., but I guess there should still be a similar space sum).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simple question: If you can sum space loads to currently give a zone load with existing code, with known space fractions of all loads, and that answer is accurate, what benefit would it be to perform sizing on spaces? Wouldn't the space sum just be a sum of the original fractions used to create a zone load? I almost get the non-coincident part but the coincident part seems to mean no space sizing is necessary (i.e., skip it). And it even seems the non-coincident sizing is just a sum of a different time in a series of data, which then would also mean no space sizing is necessary. If coincident gives the same answer as zone sizing then non-coincident just sums data from different times from that same time series data of zone sizing that is fractionally allocated to spaces. So my question is "what is meant by space sizing"? What heat balance will be performed? is it a sum of some fractions of space allocations, or is it an actual heat balance on a space? Which would then be just a subset of zone sizing with known fractions. I'm trying to think of a benefit here but not coming up with a good answer.

Lights,
  A1 , \field Name
  A2 , \field Zone or ZoneList or Space or SpaceList Name

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Space sizing is an actual heat balance on each space. Currently zone sizing is an actual heat balance on each zone (as a whole) although some of the components for the zone heat balance are sums across the spaces (even when space heat balance is off). e.g. internal gains.

For the coincident zone sizing, I was planning to just use the current lumped zone results rather than combine the space-level results into that. But I should at least check that calculating it from space results would give the same answer. It should be the same since the spaces are all using the same thermostat setpoint. Hmmm, but what if one space is floating while others are calling for cooling, or worse yet, what if one space needs heat and another needs cooling? But conceptually, the zone sizing should be the lumped result when choosing coincident space sums.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was running 5ZoneAirCooledWithSpacesHVAC last night in 24.1 and looking at results to try to get a better feel for space sizing. I see 5ZoneAirCooledWithSpacesHVACZsz.csv (and Ssz) but no 5ZoneAirCooledWithSpacesHVACSpsz for spaces so I could not see how the space temps reported during the peak time. The space temps during sizing should be at or very near the Tstat temp but if not doing latent sizing w may be very different. Adding latent sizing proved changing zone w didn't change sizing results much. I'm not sure but I wonder if the zone sizing results and the different space fractions of all loads could hold all the info needed for summing/reporting space loads. If space temps are actually different, as I expect they would be, then the heat balance is needed during the simulation but I wonder about sizing. Probably won't save much execution speed so not worth the effort. I just wanted to think through that thought a little while talking about spaces.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generating 5ZoneAirCooledWithSpacesHVACSpsz output is on the radar for this round.

Combination,
NA,
Num
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can probably combine these because Combination and NA is not a choice field in Sizing:Zone Type of Space Sum to Use.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I did that originally, but then I didn't like it. Do we have other examples of using just a subset of an enum?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the best example is the fans. Not all fan types are allowed in all parents yet there is just one enum.

fanCoil.fanType = static_cast<HVAC::FanType>(getEnumValue(HVAC::fanTypeNamesUC, Alphas(9)));

enum class FanType
{
    Invalid = -1,
    Constant,
    VAV,
    OnOff,
    Exhaust,
    ComponentModel,
    SystemModel,
    Num
};

constexpr std::array<std::string_view, (int)FanType::Num> fanTypeNames = {
    "Fan:ConstantVolume", "Fan:VariableVolume", "Fan:OnOff", "Fan:ZoneExhaust", "Fan:ComponentModel", "Fan:SystemModel"};

Copy link
Collaborator

@amirroth amirroth Jun 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's an option for working with a subset of an enum:

constexpr std::array<bool, (int)FanType::Num> fanTypeInSubset1 = {true, false, true, true, false, false};

Creating multiple enums and then mappings between them is another option, but it's not a good option in my opinion because enums are disjoint types, a variable can only be of one enum type. Better to have a single enum and then implement subsets using either arrays like this or comparison to explicit subsets in the code.

Here is another thing you can do:

constexpr std::array<FanType, 3> fanTypeSubset1 = {FanType::Constant, FanType::OnOff, FanType::Exhaust};

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fanTypeInSubset1 

Yes, go on . . .

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fanTypeInSubset1 

Yes, go on . . .

Then you can use either of the two arrays or explicit comparisons to check whether the enum value you get is in Subset1 or not.

@@ -6540,7 +6540,7 @@ void UpdateSysSizing(EnergyPlusData &state, Constant::CallIndicator const CallIn
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you did combine the enums as I suggested you would need a default here that did nothing or asserted(false).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe that's what I didn't like about a single enum.

zoneCFS.DesCoolVolFlow += spaceCFS.DesCoolVolFlow;
zoneCFS.DesCoolLoad += spaceCFS.DesCoolLoad;
zoneCFS.DesCoolMassFlow += spaceCFS.DesCoolMassFlow;
zoneCFS.DesCoolLoadNoDOAS += spaceCFS.DesCoolLoadNoDOAS;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the non-coincident case, why is the zone data getting overwritten by the space data? I understand wanting to average the space data in that case but why overwrite to zone data? If the user selects non-coincident sizing the zone sizing information will be lost. If I had seen this I would be more comfortable:

 zoneCFS.spaceDesHeatVolFlow += spaceCFS.DesHeatVolFlow;    or
 zoneSpaceCFS.DesHeatVolFlow += spaceCFS.DesHeatVolFlow;

So for the non-coincident case the data written to eplusout.zsz won't be what it was before. I thought there was going to be 2 independent reports for zone and space sizing and those reports could be compared to see how well space sizing agreed with zone sizing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might actually be OK because you could compare eplusout.zsz for the coincident and non-coincident cases and if there were no diffs, or very small diffs, then keeping both separate would not be an issue. If there are big diffs then?

Copy link
Contributor

@rraustad rraustad Jul 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see 0 diffs between coincident and non-coincident results of eplusout.zsz. Carry on (I think I got these diffs right). Not sure if this file has a difference in peak time for non-coincident but at least I see 0 diffs. There are many columns shown on the graph not shown in the legend (e.g., column B to column CC). For non-coincident I added this to the end of Sizing:Zone for each zone:

Sizing:Zone,

autosize,                !- Dedicated Outdoor Air High Setpoint Temperature for Design {C}
,
,
,
,
,
,
,
,
,
NonCoincident;            !- Type of Space Sum to Use

image

5ZoneAirCooledWithSpaceHeatBalance-epluszsz.zip

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current logic is this:

  1. Let existing code calculate the space sizing and coincident zone sizing (i.e. lump spaces together for the zone sizing).
  2. If non-coincident sizing is selected for a given zone, then override the zone sizing results based on the sums/averages of spaces in that zone.
  3. However, if the zone only has one space, then skip the override and keep the original results. That's why you didn't seen any diffs in 5ZoneAirCooledWithSpaceHeatBalance (it's only got one space per zone).
  4. Here's the zsz files for 5ZoneAirCooledWithSpacesHVAC. There are differences for zones 3 and 5. Looks like the peaks are not correct, however - the difference in peaks shouldn't be greater than the hourly diffs.
    5ZoneAirCooledWithSpacesHVAC-NonCoincident.zip

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic seems sound. I don't see the problem with non-coincident vs zone sizing as you mentioned. I was worried that overwriting data may cover up an issue but the user can always compare coincident vs non-coincident to verify results.

image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still need to figure out what to do with peak date/time. I guess I can scan the non-coincident zone seqs?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But those could have come from different design days for each space, hmm.

@mjwitte mjwitte marked this pull request as ready for review August 12, 2024 15:29
Copy link
Contributor Author

@mjwitte mjwitte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code walkthru - part 1

Comment on lines 24872 to 24873
A2, \field Zone Name
\note Name of zone that is the thermal chimney
\note Name of zone that is the thermal chimney.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the thermal chimney is a special zone, only accept a zone name here, not a space name.

@@ -24889,16 +24894,17 @@ ZoneThermalChimney,
\minimum 0
\maximum 1
\default 0.8
A4, \field Zone 1 Name
A4, \field Zone or Space Name 1
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But allow the chimney to draw from either a single space or a full zone.

@@ -34523,6 +34548,12 @@ Sizing:Zone,
\note no humidistat is associated with this zone.
\type alpha
\units percent
A15;\field Type of Space Sum to Use
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New field in Sizing:Zone for coincident/nonincident option.

@@ -50789,6 +50820,59 @@ SpaceHVAC:ZoneEquipmentMixer,
\note Matches a SpaceHVAC:EquipmentConnections Exhaust Node Name
\type node

SpaceHVAC:ZoneReturnMixer,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New object for space return air modeling.

@@ -107248,117 +107332,122 @@ OutputControl:Files,
\key Yes
\key No
\default Yes
A9 , \field Output Zone Sizing
A9 , \field Output Space Sizing
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New output file for space sizing spsz, equivalent to zsz output. This new field inserted in OutputControl:Files triggers a transition rule.

Comment on lines 640 to +641
Array1D<DataZoneEquipment::EquipConfiguration> ZoneEquipConfig;
EPVector<DataZoneEquipment::EquipConfiguration> spaceEquipConfig;
Array1D<DataZoneEquipment::EquipConfiguration> spaceEquipConfig;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make this the same type as ZoneEquipConfig so they can be passes as a parameter to a common function.

@@ -376,6 +376,9 @@ void IOFiles::OutputControl::getInput(EnergyPlusData &state)
{ // "output_audit"
audit = boolean_choice(find_input(fields, "output_audit"));
}
{ // "output_space_sizing"
spsz = boolean_choice(find_input(fields, "output_space_sizing"));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding a new output file touched a surprising number of places in the code.

} else {
c->coilSizingMethodConcurrenceName = "N/A";
}
c->coilSizingMethodConcurrenceName = DataSizing::CoilSizingConcurrenceNames[(int)c->coilSizingMethodConcurrence];
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some cleanups for the new enums.

Comment on lines +6429 to +6432
if (CoolDDNum == 0) {
auto &zoneCFS = state.dataSize->CalcFinalZoneSizing(state.dataSize->TermUnitFinalZoneSizing(TermUnitSizingIndex).ZoneNum);
OutAirTemp += zoneCFS.CoolOutTemp * coolMassFlow / (1.0 + termUnitSizing.InducRat);
OutAirHumRat += zoneCFS.CoolOutHumRat * coolMassFlow / (1.0 + termUnitSizing.InducRat);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For zone sizing that is the non-coincident total of space sizes, there isn't necessarily the same design day across all spaces, so use zone outdoor conditions here instead of design day weather data as below.

@@ -240,7 +240,16 @@ void ManageSizing(EnergyPlusData &state)
state.files.zsz.filePath = state.files.outputZszTxtFilePath;
}

if (state.dataSize->SizingFileColSep == CharComma) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More code for the new file.

Copy link
Contributor Author

@mjwitte mjwitte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code walkthru - part 2

void updateZoneSizingEndZoneSizingCalc1(EnergyPlusData &state, DataSizing::ZoneSizingData const &zsCalcSizing)
void updateZoneSizingEndZoneSizingCalc1(EnergyPlusData &state, int const zoneNum)
{
// Set or override finalzonesizing data for non-coincident sizing
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is the crux of the non-coincident sizing option. The regular old coincident sizing is always calculated. If the zone is set for non-coincident and has more than one space, then new values are calculated for non-coincident sizing which overwrite the original coincident results.

@@ -1956,23 +2304,258 @@ void updateZoneSizingEndZoneSizingCalc1(EnergyPlusData &state, DataSizing::ZoneS
"flow calculations");
}
}
zsCalcSizing.HeatPeakDateHrMin = zsCalcSizing.cHeatDDDate + ' ' + sizingPeakTimeStamp(state, zsCalcSizing.TimeStepNumAtHeatMax);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved these to a one-time call.

Comment on lines -1963 to -1965
// SpaceSizing TODO: There's gotta be a better place to set these timestamps
if (timeStepIndex == zsCalcSizing.TimeStepNumAtHeatMax) {
zsCalcSizing.HeatPeakDateHrMin = zsCalcSizing.cHeatDDDate + ' ' + format(PeakHrMinFmt, hourPrint, minutes);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not checking anymore for TimeStepNumA*Max while cycling through all of the timesteps.

return format(PeakHrMinFmt, hour, minute);
}

void writeZszSpsz(EnergyPlusData &state,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shared function to write either zsz or spsz output file.

}
}
}

// SpaceSizing TODO: For now only write zone-level zsz
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved to new function.

@@ -4540,7 +4908,7 @@ void CalcZoneMassBalance(EnergyPlusData &state, bool const FirstHVACIteration)
}

Real64 FinalTotalReturnMassFlow = 0;
CalcZoneReturnFlows(state, ZoneNum, StdTotalReturnMassFlow, FinalTotalReturnMassFlow);
zoneEquipConfig.calcReturnFlows(state, StdTotalReturnMassFlow, FinalTotalReturnMassFlow);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved to a method so this can be shared by zoneEquipConfig and spaceEquipConfig.

@@ -4726,182 +5094,6 @@ void CalcZoneMassBalance(EnergyPlusData &state, bool const FirstHVACIteration)
}
}

void CalcZoneReturnFlows(EnergyPlusData &state,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved to method in DataZoneEquipment

@@ -2898,7 +2898,7 @@ TEST_F(EnergyPlusFixture, DesiccantDehum_OnOASystemTest)
auto &finalSysSizing = state->dataSize->FinalSysSizing(1);
auto &sysSizPeakDDNum = state->dataSize->SysSizPeakDDNum(1);
EXPECT_ENUM_EQ(finalSysSizing.coolingPeakLoad, DataSizing::PeakLoad::SensibleCooling);
EXPECT_EQ(finalSysSizing.SizingOption, DataSizing::NonCoincident);
EXPECT_ENUM_EQ(finalSysSizing.SizingOption, DataSizing::SizingConcurrence::NonCoincident);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At this point, all unit test changes are simply updates to work with other codes changes (new enums, new function names, etc.).

@mjwitte
Copy link
Contributor Author

mjwitte commented Aug 19, 2024

Table diffs are due to new object/fields. e.g.
image

Otherwise, diffs are expected for 5ZoneAirCooledWithSpacesHVAC.idf because the idf has been changed by adding a SpaceHVAC:ZoneReturnMixer.

For the latest commit, not sure why there are Mac API and integration test failures.

@Myoldmopar
Copy link
Member

I'm not sure why it had failed before either. I'm lightly concerned as to why. But moving on. This looks clean now outside of needing to resolve the transition md conflict. I'll tackle that and push up the change now. This is also on the glide path so if anyone wants to hold this up, speak up.

@Myoldmopar
Copy link
Member

Okie dokie, conflict resolved and this is ready to go, I'll let CI do a little bit, but this is about to go in with a flurry of merges.

@Myoldmopar
Copy link
Member

And now this one. Yet another conflict??

@Myoldmopar
Copy link
Member

Merged on the web, I'll do a sanity check here now but this will likely merge in a few minutes.

@Myoldmopar
Copy link
Member

All happy, merging before Decent picks it back up.

@Myoldmopar Myoldmopar merged commit a59d0b6 into develop Aug 19, 2024
10 checks passed
@Myoldmopar Myoldmopar deleted the SpaceSizingHVACPart4 branch August 19, 2024 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
IDDChange Code changes impact the IDD file (cannot be merged after IO freeze) NewFeature Includes code to add a new feature to EnergyPlus
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extend Spaces to HVAC Extend Spaces to Sizing
9 participants