Skip to content

Conversation

jasondegraw
Copy link
Member

Pull request overview

This PR builds on previous work done related to linkage scheduling and adds the ability to schedule openings when using the AirflowNetwork feature.

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

Copy link

github-actions bot commented Apr 1, 2025

⚠️ Regressions detected on macos-14 for commit 5d555cb

Regression Summary
  • ERR: 35
  • EIO: 1
  • ESO Big Diffs: 1
  • Table Small Diffs: 1

May need to go back in here and adjust the schedule further to
get closer to the original result, this is changing the results
quite a bit. Maybe a Schedule:File that mimics the original
behavior?
Copy link

github-actions bot commented Apr 1, 2025

⚠️ Regressions detected on macos-14 for commit 74da9d4

Regression Summary
  • Audit: 1
  • EIO: 2
  • ERR: 35
  • ESO Big Diffs: 2
  • Table Big Diffs: 1
  • Table Small Diffs: 1

int RAFNNodeNum; // Index of RAFN node number

// Default Constructor
MultizoneZoneProp()
Copy link
Collaborator

Choose a reason for hiding this comment

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

I thought we're not doing constructors this way anymore and we're putting the default values into the declarations. That's much cleaner IMO. Same comment on constructors below.

{
// Members
std::string ZoneName; // Name of Associated EnergyPlus Thermal Zone
std::string VentControl; // Ventilation Control Mode: "TEMPERATURE", "ENTHALPIC", "CONSTANT", or "NOVENT"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this a string?

// and moisture levels at each node, and airflow and sensible and latent energy losses
// at each element

static Real64 square(Real64 x)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why add the function call overhead? Will this inline if you add the inline keyword?


namespace AirflowNetwork {

void handle_nonrectangular_surfaces(EquivRec equivalent_rectangle_method, EPVector<DataSurfaces::SurfaceData> Surface, const std::string &surface_name, int surface_number,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Passing EPVector by value? Not const &?

void handle_nonrectangular_surfaces(EquivRec equivalent_rectangle_method, EPVector<DataSurfaces::SurfaceData> Surface, const std::string &surface_name, int surface_number,
Real64 &height, Real64 &width, Real64 user_aspect_ratio, EnergyPlusData *state)
{
if (equivalent_rectangle_method == EquivRec::Height) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

auto &surf = Surface(surface_number); shortcut would make this code more readable.

auto const equiv_rect_iter{fields.find("equivalent_rectangle_method")};
EquivRec equivrec{EquivRec::Height};
if (equiv_rect_iter != fields.end()) {
// This is guaranteed to work since inputs are validated
Copy link
Collaborator

Choose a reason for hiding this comment

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

getEnumValue, not maps.

"\" is an air boundary surface.");
ShowContinueError(m_state, "Ventilation Control Mode = " + Alphas(4) + " is not valid. Resetting to Constant.");
MultizoneSurfaceData(i).VentSurfCtrNum = VentControlType::Const;
ShowContinueError(m_state, "Ventilation Control Mode = " + std::map<VentControlType, std::string> {{VentControlType::Temp, "TEMPERATURE"},
Copy link
Collaborator

@amirroth amirroth Apr 3, 2025

Choose a reason for hiding this comment

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

Once more. Creating a single-use map and then searching it is slower than just doing the if-else string comparison ladder that was there before.

@@ -5170,6 +4954,20 @@ namespace AirflowNetwork {
}
}

// Final linkage validation
for (auto& linkage : AirflowNetworkLinkageData)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can be auto const &.

int LF;
int LT;
int CompNum;
int NF;
int NT;
iComponentTypeNum CompTypeNum;
AirflowElementType CompTypeNum;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why remove num from the type name but not the variable name?


// The original test was changing the CompTypeNum, as that goes away it's necessaey to actually
// switch out the elements. This is probably an unwise approach.
auto const ye_olde_element = state->afn->AirflowNetworkLinkageData(2).element;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since element is a pointer, please declare this as auto const *.

@nrel-bot-2
Copy link

@jasondegraw @Myoldmopar it has been 30 days since this pull request was last updated.

@nrel-bot-2c
Copy link

@jasondegraw @Myoldmopar it has been 31 days since this pull request was last updated.

@nrel-bot-2c
Copy link

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

@nrel-bot-2c
Copy link

@jasondegraw @Myoldmopar it has been 30 days since this pull request was last updated.

@nrel-bot-2c
Copy link

@jasondegraw @Myoldmopar it has been 28 days since this pull request was last updated.

@nrel-bot-2
Copy link

@jasondegraw @Myoldmopar it has been 30 days since this pull request was last updated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants