Skip to content

Conversation

mjwitte
Copy link
Contributor

@mjwitte mjwitte commented Mar 18, 2025

Pull request overview

Description of the purpose of this PR

Pull Request Author

  • 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

  • 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 Defect Includes code to repair a defect in EnergyPlus OutputChange Code changes output in such a way that it cannot be merged after IO freeze labels Mar 18, 2025
Copy link

⚠️ Regressions detected on macos-14 for commit 21dc48f

Regression Summary
  • Table Big Diffs: 736
  • Table String Diffs: 735

@mjwitte mjwitte added this to the EnergyPlus 25.2 IO Freeze milestone Mar 18, 2025
@mjwitte
Copy link
Contributor Author

mjwitte commented Mar 18, 2025

Here is a sample of the current table for OutdoorAirUnit.idf.
image

Here are some samples of the revised table. Note the changes from FlatPlate to Plate, and from [kg/s] to [m3/s].

DesiccantDehumidifierWithCompanionCoil
image

ASHRAE901_HotelLarge_STD2019_Denver
image

OutdoorAirUnit
image

int CoolEffectLatentCurveIndex = 0; // cooling latent effectiveness multiplier curve to CoolEffectLatent100
// 1 = None, 2 = Bypass, 3 = Stop Rotary HX Rotation
HXConfigurationType ExchConfig = HXConfigurationType::Invalid; // parameter equivalent of HX configuration, plate or rotary
HXConfigurationType ExchConfig = HXConfigurationType::Rotary; // parameter equivalent of HX configuration, plate or rotary
Copy link
Contributor

Choose a reason for hiding this comment

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

Not standard but I assume this was done for the sake of the desiccant HX (as opposed to leaving this as invalid and setting Rotary in the desiccant HX getInput). If no other changes, which I don't expect since these changes and results look good, leave as is.

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 was a punt to fix a unit test (maybe more than one?) that didn't do getInput and was throwing a vector error when trying to fill the table in size. I can undo this and set it properly in the unit test and add some asserts to protect that.

Copy link
Contributor

Choose a reason for hiding this comment

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

This change would be good to get into the latest release so I figured it could go as-is. There's still a few days left so it's up to you how you handle clean-up changes.

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 there are OutputChanges (delete duplicate Name column, and change units on two columns), so I'm not sure this will be allowed in for 25.1. @Myoldmopar?

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, then you have plenty of time.

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 also consider whether to change "Outdoor Airflow" to "Supply AirFlow".

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 can also consider whether to change "Outdoor Airflow" to "Supply AirFlow".

Hmm, so they're currently called "Exhaust" and "Outdoor", but the three HXs all use different terminology:
HeatExchanger:AirToAir:FlatPlate: Supply and Secondary
HeatExchanger:AirToAir:SensibleAndLatent: Supply and Exhaust
HeatExchanger:Desiccant:BalancedFlow: Process and Regeneration

@JasonGlazer Any preference?

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be good to use a consistent set of terms. I think AHRI probably has a standard on this, maybe we should see what terms it uses.

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 would be good to use a consistent set of terms. I think AHRI probably has a standard on this, maybe we should see what terms it uses.

Supply and Exhaust, based on these two docs:
https://www.airxchange.com/wp-content/uploads/2019/07/AHRI-Guideline-W-2005.pdf
https://www.airxchange.com/wp-content/uploads/2019/07/AHRI-Standard-1060-I-P-2013.pdf

Copy link

⚠️ Regressions detected on macos-14 for commit 17347e0

Regression Summary
  • Audit: 794

@mjwitte
Copy link
Contributor Author

mjwitte commented Mar 18, 2025

An updated example table.
image

@rraustad
Copy link
Contributor

rraustad commented Mar 18, 2025

That looks very good. Why 0.63 and 0.43 in lower right corner? Never mind, that's the HX with both inputs.

@rraustad
Copy link
Contributor

I just noticed you got rid of the duplicate name column. Put the header column title back as "Name" ?

@rraustad
Copy link
Contributor

rraustad commented Mar 18, 2025

And maybe change the title "Input object type" to just "Type" like other tables.

@mjwitte mjwitte marked this pull request as ready for review March 19, 2025 01:58
@JasonGlazer
Copy link
Contributor

Looking good!

@mjwitte
Copy link
Contributor Author

mjwitte commented Mar 20, 2025

I just noticed you got rid of the duplicate name column. Put the header column title back as "Name" ?

Well, most table reports have no heading in the first column.
image

I suppose we could change that. @JasonGlazer is there a way to place a header in the first column with current functions?

And maybe change the title "Input object type" to just "Type" like other tables.
Ok, done (locally).

@mjwitte
Copy link
Contributor Author

mjwitte commented Mar 20, 2025

From #10985 (comment)

I'll go with repeating the nominal supply flow, unless you object.

Can we report the maximum flow also?

@JasonGlazer Is this still something we want here? It would be a simulation result rather than documenting an input.

@JasonGlazer
Copy link
Contributor

I suppose we could change that. @JasonGlazer is there a way to place a header in the first column with current functions?

I don't think there is a way to do that, but it could be added if it is important. newPreDefSubTable() could have another optional argument that would be the first column heading. I have thought about this before but decided in the past that it probably didn't matter too much.

@JasonGlazer
Copy link
Contributor

@JasonGlazer Is this still something we want here? It would be a simulation result rather than documenting an input.

Results are welcome!

Copy link

⚠️ Regressions detected on macos-14 for commit 45a01d7

Regression Summary
  • Audit: 794

@mjwitte
Copy link
Contributor Author

mjwitte commented Mar 21, 2025

Here's the latest version of the table (for OutdoorAirUnit.idf):
image

This is ready for final review after v25.1 is released. (The output changes will need to be moved the the 25.2 file at some point).

Copy link

github-actions bot commented Apr 9, 2025

⚠️ Regressions detected on macos-14 for commit 351897c

Regression Summary
  • Audit: 794

@Myoldmopar Myoldmopar self-assigned this Apr 9, 2025
@nrel-bot-2
Copy link

@Myoldmopar @mjwitte @Myoldmopar it has been 29 days since this pull request was last updated.

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.

All good here. Will run some quick local testing, but expect this to merge shortly.

Invalid = -1,
AirToAir_FlatPlate,
AirToAir_Generic,
AirToAir_SensAndLatent,
Copy link
Member

Choose a reason for hiding this comment

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

That is a bit nicer naming.

CompName = thisBDDPerf.Name;
CompType = thisBDDPerf.PerfType;
SizingString = thisBDDPerf.NumericFieldNames(FieldNum) + " [m3/s]";
TempSize = thisBDDPerf.NomSupAirVolFlow;
Copy link
Member

Choose a reason for hiding this comment

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

Nice and readable here.

OutputReportPredefined::PreDefTableEntry(state, state.dataOutRptPredefined->pdchAirHRSenEffAt100PerCoolAirFlow, this->Name, "N/A");
OutputReportPredefined::PreDefTableEntry(state, state.dataOutRptPredefined->pdchAirHRLatEffAt100PerHeatAirFlow, this->Name, "N/A");
OutputReportPredefined::PreDefTableEntry(state, state.dataOutRptPredefined->pdchAirHRLatEffAt100PerCoolAirFlow, this->Name, "N/A");
}
Copy link
Member

Choose a reason for hiding this comment

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

Good output report changes here.

OutputReportPredefined::RetrievePreDefTableEntry(*state, state->dataOutRptPredefined->pdchAirHRLatEffAt100PerCoolAirFlow, compName));

EXPECT_EQ("1.00", OutputReportPredefined::RetrievePreDefTableEntry(*state, state->dataOutRptPredefined->pdchAirHRSupplyAirflow, compName));
EXPECT_EQ("1.00", OutputReportPredefined::RetrievePreDefTableEntry(*state, state->dataOutRptPredefined->pdchAirHRExhaustAirflow, compName));
Copy link
Member

Choose a reason for hiding this comment

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

👍

@Myoldmopar
Copy link
Member

Needed to resolve conflicts just from the output changes file. I'll let CI run, but this is all happy locally and should be good to merge shortly.

Copy link

⚠️ Regressions detected on macos-14 for commit 39cd0f7

Regression Summary
  • Audit: 794
  • Table Big Diffs: 736

@Myoldmopar
Copy link
Member

Alright, this seems ready to go now. CI is happy, and I just did a sanity check right now. Thanks @mjwitte !

@Myoldmopar Myoldmopar merged commit 7453159 into develop May 28, 2025
9 checks passed
@Myoldmopar Myoldmopar deleted the rpdTables1 branch May 28, 2025 13:54
@mjwitte mjwitte added the RPD label Aug 4, 2025
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 OutputChange Code changes output in such a way that it cannot be merged after IO freeze RPD

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Equipment Summary - Air Heat Recovery table values

6 participants