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

Add Reports to Support createRMD - ruleset model description #10372

Merged
merged 95 commits into from
Jul 12, 2024

Conversation

JasonGlazer
Copy link
Contributor

Pull request overview

  • Support for the next phase of createRMD development by adding reports and the NFP

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

@JasonGlazer JasonGlazer added the NewFeature Includes code to add a new feature to EnergyPlus label Jan 18, 2024
JasonGlazer and others added 22 commits February 2, 2024 10:12
…Pump:AirToAir and AirLoopHVAC:UnitaryHeatPump:WaterToAir
@JasonGlazer JasonGlazer marked this pull request as ready for review July 2, 2024 12:13
@Myoldmopar Myoldmopar assigned Myoldmopar and unassigned JasonGlazer Jul 3, 2024
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 is a huge set of changes. I actually tried to get through them all. Looks like some pretty standard patterns being applied, you've documented it well, and lots of added testing including 2 new unit test files. I have a couple questions/comments, but I suspect this is very close.

@@ -3427,7 +3427,7 @@ namespace CondenserLoopTowers {
this->Name,
state.dataPlnt->PlantLoop(this->plantLoc.loopNum).FluidName); // Fluid Name more reasonable than FluidType
OutputReportPredefined::PreDefTableEntry(state, state.dataOutRptPredefined->pdchCTFCRange, this->Name, this->DesignRange);
OutputReportPredefined::PreDefTableEntry(state, state.dataOutRptPredefined->pdchCTFCRange, this->Name, this->DesignApproach);
OutputReportPredefined::PreDefTableEntry(state, state.dataOutRptPredefined->pdchCTFCApproach, this->Name, this->DesignApproach);
Copy link
Member

Choose a reason for hiding this comment

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

👍

{
// Report the layers for each opaque construction in predefined tabular report
// J. Glazer March 2024
auto &opd = state.dataOutRptPredefined;
Copy link
Member

Choose a reason for hiding this comment

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

I don't think you ever used this shortcut you made. I'd say either remove it or use it in the loop. In this tiny little function with just a couple uses, it's fine either way. Also maybe I just can't see the usage here on GH.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I removed that line.

if (state.dataOutRptPredefined->pdchOpqConsLayCol.size() > 0) {
for (int i = 1; i <= this->TotLayers; ++i) {
int layerIndex = this->LayerPoint(i);
auto const *thisMaterial = state.dataMaterial->Material(layerIndex);
Copy link
Member

Choose a reason for hiding this comment

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

Hmm... are materials held as pointers? If so, this is great. If not, this is still technically right, but you could use a reference instead like we do all over. I'm assuming it must be held as a pointer or you wouldn't have done that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I just copied that line without thinking. I fixed it to a normal reference.

@@ -85,6 +85,9 @@ namespace DataAirLoop {
Array1D_int TermUnitCoolSizingIndex; // Air terminal sizing numbers for zones cooled by this air loop
Array1D_int TermUnitHeatSizingIndex; // Air terminal sizing numbers for zones heated by this air loop
Array1D<HVAC::AirDuctType> SupplyDuctType; // 1=main, 2=cooling, 3=heating, 4=other
Array1D_int SupplyDuctBranchNum; // Supply duct branch number
Array1D_int SupplyAirPathNum; // Supply air path indexes
Array1D_int ReturnAirPathNum; // Return air path indexes
Copy link
Member

Choose a reason for hiding this comment

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

OK, but I still think EPVector is a better option for a lightweight 1-based array. Both Array1D and EPVector are destined to become regular old C++ vectors, so it's not crucial, but @jasondegraw and company have set up EPVector as a great piece of that transition.

Anyway, no huge deal, carrying on...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using EPVector is a good idea here.

@@ -2386,8 +2397,8 @@ void FanComponent::report(EnergyPlusData &state)
totalEnergy = totalPower * state.dataHVACGlobal->TimeStepSysSec;
deltaTemp = outletAirTemp - inletAirTemp;

if (type == HVAC::FanType::OnOff) {
if (airLoopNum > 0) {
if (isAFNFan && (airLoopNum > 0)) {
Copy link
Member

Choose a reason for hiding this comment

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

Is isAFNFan another bug fix?

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 don't remember @mjwitte do you?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is isAFNFan another bug fix?

Not really. Previously airLoopNum was being used (abused) only for AFN fans. So, the code assumed that airloopNum>0 meant this fan was an AFN fan. When I expanded the use of airLoopNum for be set for all fans (to support RMD reports) that was no longer the case, so I added the new isAFNFan flag to track that.

@@ -688,6 +689,8 @@ namespace Furnaces {
std::string_view constexpr getAirLoopHVACHeatCoolInput("GetAirLoopHVACHeatCoolInput");
std::string_view constexpr routineName = "GetFurnaceInput";

using namespace OutputReportPredefined;
Copy link
Member

Choose a reason for hiding this comment

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

Is this needed? You are explicitly listing out the namespace when you are calling PreDefTableEntry, which is our current pattern, so I'm not sure this is needed.

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 agree.

@@ -1479,8 +1486,10 @@ void InitAirLoops(EnergyPlusData &state, bool const FirstHVACIteration) // TRUE
}
EPVector<int> supNode;
EPVector<DataZoneEquipment::AirNodeType> supNodeType;
EPVector<int> supNodeCompNum;
Copy link
Member

Choose a reason for hiding this comment

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

👍

\item
Exterior Fenestration which includes all non-opaque exterior surfaces and includes the name of the construction, areas (glass, frame, divider, single opening, multiplied openings), glass U-Factor, glass SHGC (the solar heat gain coefficient based on summer conditions), glass visible transmittance, NFRC Product Type, assembly U-Factor, assembly SHGC, assembly visible transmittance, conductance (frame, divider), indication of shade control, the name of the parent surface, azimuth, tilt, cardinal direction.
Opaque exterior which includes all exterior opaque surfaces and includes the name of the construction, zone, adjacent surface, reflectance, U-Factors, areas, azimuth, tilt, cardinal direction.
Copy link

Choose a reason for hiding this comment

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

Is there a way to indicate whether the surface is modeled as casting shade on other exterior surfaces?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean like the report that is in the .SHD output file?

\item
Interior Fenestration which includes the construction, areas, glass u-factor, SHGC, and visible transmittance, and the parent surface name.
\item
Exterior Door which includes the construction, u-factors, area, and the parent surface name.
Copy link

Choose a reason for hiding this comment

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

I know door type (swinging, nonswinging) and glazed/opaque area are less of EnergyPlus issue but more a compliance issue. Are there a post processing logic to map these two data?
Also, could we map the Surface Type to fenestration surfaces? - at least we can know if the surface is a door or glazed door.

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 don't think there is enough information to figure out swinging/nonswining for doors so it would have to be something that the user provides as a compliance parameter. I didn't actually touch this table on exterior doors I just noticed that it wasn't documented but from my code review it doesn't seem like GlassDoors are being reported here I think they end up being reported as exterior fenestration.

@@ -73,11 +73,22 @@ \subsubsection{Predefined Annual Summary Reports}\label{predefined-annual-summar

\begin{itemize}
\item
Opaque which includes all opaque surfaces and includes the name of the construction, reflectance, U-Factor, gross area, azimuth, tilt, cardinal direction.
Opaque exterior which includes all exterior opaque surfaces and includes the name of the construction, zone, reflectance, U-Factors, areas, azimuth, tilt, cardinal direction.
Copy link

Choose a reason for hiding this comment

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

Is this line describe the same table as the line 78?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for catching this, the second line is supposed to be for opaque interior surfaces.

\item
Heating Coils includes the nominal capacity and efficiency for each heating coil. The capacity is calculated by calling the heating coil simulation routine at the rated inlet conditions: inlet air dry bulb temperature = 16.6C, inlet relative humidity = 50\%, inlet hot water temperature = 82.2C.
\item
Fan includes the type of fan, the total efficiency, delta pressure, max flow rate, motor heat in air fraction, and end use.
Fan includes the type of fan, the total efficiency, delta pressure, max flow rate, rated electricity rate, power per flow, fan energy index, motor heat in air fraction, end use, design day for fan sizing peak and date and time, fan purpose, if the fan is autosized, the motor efficiency, fraction of motor heat to the zone and the name of the zone, airloop name.
Copy link

Choose a reason for hiding this comment

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

I assume the data relevant to fan controls will be extracted from the IDFs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are hoping to add controls in a future reporting update which would include fan controls.

\item
Cooling Towers and Fluid Coolers show the type, fluid type, range and approahc, design fan power, inlet web-bulb temperature, flow rate, leaving water setpoint temperature, and the name of the condenser loop and branch.
\item
PlantLoop or CondenserLoop show the type, if it provides heating or cooling, the maximum and minimum loop flow rate
Copy link

Choose a reason for hiding this comment

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

I think the difficulty for plant loop is to identify parent and child. Do you think it is possible for Eplus to provide something such as connected loop?
The logic can be when parent's demand side connect to the supply side of other loops.
However, I also think this can be done outside of the reporting as well - whichever is more efficient in terms of development will work for me.

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 think the new topology report should address the way components are connected.

@@ -207,6 +207,17 @@ namespace DataSizing {
Num
};

constexpr std::array<std::string_view, static_cast<int>(SysOAMethod::Num)> SysOAMethodNames{"Zone Sum",
Copy link

Choose a reason for hiding this comment

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

Does this array needs a string for BLANK?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is matching an existing enumeration that did not have a "None" or "Blank" so the code must not allow that option.

@JasonGlazer
Copy link
Contributor Author

I believe I addressed all the comments. I hope the CI comes back clean and this will be ready to merge.

@Myoldmopar
Copy link
Member

Looks like all 5 CI tests are underway, so we'll know soon...

@Myoldmopar
Copy link
Member

Lots of table, eio, and audit diffs, but I spot checked things locally and they look good. New tables and new columnar data look to match up with the output changes description already here. This seems ready to merge. Thanks @JasonGlazer

This potentially will cause diffs in other branches, so any other reviewers, be aware of that this morning.

@Myoldmopar Myoldmopar merged commit 4abffaf into develop Jul 12, 2024
15 checks passed
@Myoldmopar Myoldmopar deleted the Support-CreateRMD-Ph3 branch July 12, 2024 14:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NewFeature Includes code to add a new feature to EnergyPlus OutputChange Code changes output in such a way that it cannot be merged after IO freeze
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants