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

Mean Radiant Temperature from Zone to Enclosure #10244

Merged
merged 31 commits into from Nov 21, 2023
Merged

Conversation

mjwitte
Copy link
Contributor

@mjwitte mjwitte commented Sep 26, 2023

Pull request overview

  • Fixes Mean Radiant Temperature (MRT) calculations are zone-based (not enclosure-based) #9377 (remaining pieces)
  • Builds on Calculate Mean Radiant Temperature (MRT) for Enclosures (not zones) #9628
  • Delete unused "Zone Name" field from ComfortViewFactorAngles
  • Change People "Mean Radiant Temperature Calculation Type" option from "ZoneAveraged" to "EnclosureAvereged"
  • Change "Watts per Zone Floor Area" fields to "Watts per Floor Area" in Lights, ElectricEquipment, and ElectricEquipment:ITE:AirCooled (not really related to MRT, but noticed it here).
  • Change "Power per Zone Floor Area" to "Power per Floor Area" in GasEquipment, HotWaterEquipment, and SteamEquipment.
  • Clean up CalcSurfaceWeightedMRT which was already using enclosures, but change from zone to space MAT as the fallback when the sum of surface areas*emissivities is zero.
  • Move loose ZoneMRT array to MRT field in zone/spaceHeatBalance struct.
  • Add new output variables for "Space Mean Radiant Temperature" and "Enclosure Mean Radiant Temperature".
  • Use enclosure MRT for interzone windows.
  • Use enclosure MRT for Space Operative Temperature (Zone Operative Temperature is still using Zone MRT).
  • Use enclosure MRT for thermal comfort (since People instances are all at the space level).

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 the Defect Includes code to repair a defect in EnergyPlus label Sep 26, 2023
@mjwitte mjwitte added this to the EnergyPlus 24.1 IOFreeze milestone Sep 26, 2023
@nealkruis nealkruis changed the title Meant Radiant Temperature from Zone to Enclosure Mean Radiant Temperature from Zone to Enclosure Sep 27, 2023
ShowContinueError(state, "As a result, MRT will be set to MAT for that zone");
}
state.dataHeatBal->ZoneMRT(ZoneNum) = state.dataZoneTempPredictorCorrector->zoneHeatBalance(ZoneNum).MAT;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I am staring at this if/else and wondering what the for loop at lines 5488 - 5495 represents when ZoneAESum(ZoneNum) <= 0.01. Same for the enclosure loop below at 5511. If ZoneAESum(ZoneNum) <= 0.01, then isn't sumAE also <= 0.01? And doesn't that mean that the for loop could be moved into this if block above line 5497? The only draw-back I see is the calculation of state.dataViewFactor->EnclRadInfo(state.dataSurface->Surface(SurfNum).RadEnclIndex).sumAET would only occur when ZoneAESum > 0.01.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rraustad Good point - no sense in accumulating sumAET if it's not going to be used, because sumAE is zero.

Copy link
Member

Choose a reason for hiding this comment

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

Does this mean something still needs to be cleaned out 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.

@Myoldmopar The comment was addressed. if (state.dataHeatBalSurfMgr->ZoneAESum(ZoneNum) > 0.01) { was moved up to line 5492, before the loop to accumulate zoneSumAET.

Copy link
Member

Choose a reason for hiding this comment

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

Excellent, thanks!

@mjwitte mjwitte added the IDDChange Code changes impact the IDD file (cannot be merged after IO freeze) label Oct 3, 2023
@mjwitte mjwitte marked this pull request as ready for review October 5, 2023 19: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. This is ready for final review.

Comment on lines +389 to +390
\paragraph{Space Mean Radiant Temperature {[}C{]}}\label{space-mean-radiant-temperature-c}
\paragraph{Enclosure Mean Radiant Temperature {[}C{]}}\label{enclosure-mean-radiant-temperature-c}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Two new output variables. Note that Zone MRT is still calculated the legacy way, oblivious to enclosure boundaries.

\key ZoneAveraged
\key EnclosureAveraged
Copy link
Contributor Author

Choose a reason for hiding this comment

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

key change for People. This now uses the enclosure MRT for the associated space. No longer using zone MRT.

Copy link
Member

Choose a reason for hiding this comment

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

Key change, people!

image

Comment on lines -22222 to +22218
\note Watts/Area => Watts per Zone Floor Area -- enter the number to apply. Value * Floor Area = Lights
\note Watts/Area => Watts per Floor Area -- enter the number to apply. Value * Floor Area = Lights
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some unrelated field name cleanup.

@@ -1913,7 +1916,6 @@ struct HeatBalanceData : BaseGlobalStruct
Array1D<Real64> ZoneGroupSNLoadHeatRate;
Array1D<Real64> ZoneGroupSNLoadCoolRate;

Array1D<Real64> ZoneMRT; // MEAN RADIANT TEMPERATURE (C)
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 this into zoneHeatBalance.MRT (and space.....)

Comment on lines +93 to +96
Real64 sumAE = 0.0; // Sum of surface area * emissivity for all surfaces in the enclosure
Real64 sumAET = 0.0; // Sum of surface area * emissivity * temperature for all surfaces in the enclosure
Real64 MRT = 0.0; // Mean radiant temperature of the enclosure
bool reCalcMRT = false; // True when MRT needs to be recalculated
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 enclosure fields.

thisAngFacList.SurfaceName(SurfNum) = state.dataIPShortCut->cAlphaArgs(SurfNum + 2);
thisAngFacList.SurfaceName(SurfNum) = state.dataIPShortCut->cAlphaArgs(SurfNum + 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.

Shift fields for ComfortViewFactorAngles

Comment on lines +2231 to +2233
case DataHeatBalance::CalcMRT::EnclosureAveraged: {
int enclNum = state.dataHeatBal->space(thisPeople.spaceIndex).radiantEnclosureNum;
state.dataThermalComforts->RadTemp = state.dataViewFactor->EnclRadInfo(enclNum).MRT;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here's the change from zone to enclosure MRT for people.

Copy link
Member

Choose a reason for hiding this comment

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

"So why didn't this PR just change this one line?", he asks as he runs away laughing maniacally.

tsky = state.dataHeatBal->ZoneMRT(ZoneNumAdj) +
tsky = state.dataViewFactor->EnclRadInfo(enclNumAdj).MRT +
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Use enclosure MRT for interzone windows.

! So instead, let's just move forward with a new 4 character string and use that in this file and the future
! If we get to version 100.1 and we are still using this Fortran transition then well....we can deal with it then
sVersionNum = '***'
sVersionNumFourChars='23.2'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the transition code template. @Myoldmopar this might break the version update script?

Copy link
Member

Choose a reason for hiding this comment

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

No it should be fine I think.

@@ -5841,6 +5846,77 @@ TEST_F(EnergyPlusFixture, HeatBalanceIntRadExchange_SetupEnclosuresWithAirBounda
EXPECT_EQ(state->dataHeatBal->space(1).HTSurfaceLast, Zone1Floor);
EXPECT_EQ(state->dataHeatBal->space(2).HTSurfaceLast, Zone2Floor);
EXPECT_EQ(state->dataHeatBal->space(3).HTSurfaceLast, Zone3Floor);

// Check MRT calculations
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Extend existing unit test to check MRT calcs. All other unit test changes are just to keep things working.

@mjwitte
Copy link
Contributor Author

mjwitte commented Oct 12, 2023

err diffs are new enclosure warning in a few files:

+   ** Warning ** Enclosure areas*inside surface emissivities are summing to zero, for Enclosure="ZONE ONE"
+   **   ~~~   ** As a result, MRT will be set to the volume weighted average MAT for that enclosure

AUD, RDD, MTD diffs due to new output variables.

eso small diffs in PMV, likely due to changes in CalcZoneMRT function.

@mjwitte mjwitte assigned Myoldmopar and unassigned mjwitte Nov 9, 2023
Copy link
Member

@Myoldmopar Myoldmopar left a comment

Choose a reason for hiding this comment

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

This looks great to me! There is one comment that seems to keep the door open for more changes, but I think it's for another day. I'm testing locally to confirm but expect this to be ready to merge.

\key ZoneAveraged
\key EnclosureAveraged
Copy link
Member

Choose a reason for hiding this comment

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

Key change, people!

image

\type object-list
\object-list AllHeatTranSurfNames
N99, \field Angle Factor 99
\type real
\minimum 0.0
\maximum 1.0
A102, \field Surface 100 Name
A101, \field Surface 100 Name
Copy link
Member

Choose a reason for hiding this comment

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

That's a lot of damage just for a field removal, but such is life!

@@ -12805,7 +12805,8 @@ namespace AirflowNetwork {
Tcomfort = CurveValue(state, ComfortHighTempCurveNum, OutDryBulb);
}
ComfortBand = -0.0028 * (100 - MaxPPD) * (100 - MaxPPD) + 0.3419 * (100 - MaxPPD) - 6.6275;
Toperative = 0.5 * (state.dataZoneTempPredictorCorrector->zoneHeatBalance(ZoneNum).MAT + state.dataHeatBal->ZoneMRT(ZoneNum));
Toperative = 0.5 * (state.dataZoneTempPredictorCorrector->zoneHeatBalance(ZoneNum).MAT +
Copy link
Member

Choose a reason for hiding this comment

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

Someday I wonder if we should rename the dataZoneTempPredictorCorrector member to something smaller. Would the increase in readability throughout the code make it worth the diffs? I think so actually. But obviously not here.

void calcMeanAirTemps(EnergyPlusData &state,
Real64 const ZTAV,
Real64 const airHumRatAvg,
Real64 const MRT,
Copy link
Member

Choose a reason for hiding this comment

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

👍

ShowContinueError(state, "As a result, MRT will be set to MAT for that zone");
}
state.dataHeatBal->ZoneMRT(ZoneNum) = state.dataZoneTempPredictorCorrector->zoneHeatBalance(ZoneNum).MAT;
}
Copy link
Member

Choose a reason for hiding this comment

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

Does this mean something still needs to be cleaned out here?

Comment on lines +2231 to +2233
case DataHeatBalance::CalcMRT::EnclosureAveraged: {
int enclNum = state.dataHeatBal->space(thisPeople.spaceIndex).radiantEnclosureNum;
state.dataThermalComforts->RadTemp = state.dataViewFactor->EnclRadInfo(enclNum).MRT;
Copy link
Member

Choose a reason for hiding this comment

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

"So why didn't this PR just change this one line?", he asks as he runs away laughing maniacally.

! So instead, let's just move forward with a new 4 character string and use that in this file and the future
! If we get to version 100.1 and we are still using this Fortran transition then well....we can deal with it then
sVersionNum = '***'
sVersionNumFourChars='24.1'
Copy link
Member

Choose a reason for hiding this comment

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

...... how was this not already in there? That's .. ummm .. weird.

! So instead, let's just move forward with a new 4 character string and use that in this file and the future
! If we get to version 100.1 and we are still using this Fortran transition then well....we can deal with it then
sVersionNum = '***'
sVersionNumFourChars='23.2'
Copy link
Member

Choose a reason for hiding this comment

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

No it should be fine I think.


# Object Change: ObjectStartsWithP
[PR#10244](https://github.com/NREL/EnergyPlus/pull/10244/)
Copy link
Member

Choose a reason for hiding this comment

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

Excellent instructions here. Thanks!

@Myoldmopar
Copy link
Member

This is all good locally, getting this merged now. Thanks @mjwitte !

@Myoldmopar Myoldmopar merged commit d1dfe02 into develop Nov 21, 2023
15 checks passed
@Myoldmopar Myoldmopar deleted the 9377MRTPart2 branch November 21, 2023 17:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Defect Includes code to repair a defect in EnergyPlus IDDChange Code changes impact the IDD file (cannot be merged after IO freeze)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mean Radiant Temperature (MRT) calculations are zone-based (not enclosure-based)
8 participants