Skip to content

add trap to not process population concurrence unless VRP present#6489

Merged
Myoldmopar merged 5 commits intodevelopfrom
6487-population-diversity-follow-up
Mar 17, 2018
Merged

add trap to not process population concurrence unless VRP present#6489
Myoldmopar merged 5 commits intodevelopfrom
6487-population-diversity-follow-up

Conversation

@EnergyArchmage
Copy link
Copy Markdown
Contributor

@EnergyArchmage EnergyArchmage commented Feb 21, 2018

follow up for performance issue for standard 62.1 changes from #6372

Pull request overview

Please change this line to a description of the pull request, with useful supporting information including whether it is a new feature, or fixes a defect, a cross reference to any defects addressed in this PR, the type of changes to be made, and whether diffs are expected/justified based on this change.

Work Checklist

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)
  • At least one of the following appropriate labels must be added to this PR to be consumed into the changelog:
    • Defect: This pull request repairs a github defect issue. The github issue should be referenced in the PR description
    • Refactoring: This pull request includes code changes that don't change the functionality of the program, just perform refactoring
    • NewFeature: This pull request includes code to add a new feature to EnergyPlus
    • Performance: This pull request includes code changes that are directed at improving the runtime performance of EnergyPlus
    • DoNoPublish: This pull request includes changes that shouldn't be included in the changelog

Review Checklist

This will not be exhaustively relevant to every PR.

  • Code style (parentheses padding, variable names)
  • Functional code review (it has to work!)
  • If defect, results of running current develop vs this branch should exhibit the fix
  • CI status: all green or justified
  • Performance: CI Linux results include performance check -- verify this
  • Unit Test(s)
  • C++ checks:
    • Argument types
    • If any virtual classes, ensure virtual destructor included, other things
  • IDD changes:
    • Verify naming conventions and styles, memos and notes and defaults
    • Open windows IDF Editor with modified IDD to check for errors
    • If transition, add rules to spreadsheet
    • If transition, add transition source
    • If transition, update idfs
  • If new idf included, locally check the err file and other outputs
  • Documentation changes in place
  • Changed docs build successfully
  • ExpandObjects changes?
  • If output changes, including tabular output structure, add to output rules file for interfaces

@EnergyArchmage EnergyArchmage added the DoNotPublish Includes changes that shouldn't be reported in the changelog label Feb 24, 2018
rraustad
rraustad reviewed Feb 26, 2018
View reviewed changes
src/EnergyPlus/SizingManager.cc
bool anyVRPinModel( false );
for ( int AirLoopNum = 1; AirLoopNum <= NumPrimaryAirSys; ++AirLoopNum ) {
if ( FinalSysSizing( AirLoopNum ).SystemOAMethod == SOAM_VRP ) {
anyVRPinModel = true;
Copy link
Copy Markdown
Collaborator

@rraustad rraustad Feb 26, 2018

Choose a reason for hiding this comment

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

could break here

@mjwitte mjwitte added this to the EnergyPlus 8.9.0 milestone Mar 2, 2018
@Myoldmopar
Copy link
Copy Markdown
Member

Myoldmopar commented Mar 16, 2018

@EnergyArchmage did you expect to see diffs like this produced? I checked the absdiff table results and it shows there is a difference of 1.0 on the population diversity value. Please confirm. Also, I agree with @rraustad that since this is a performance improvement, you might as well break early on that loop when you calculate anyVRPinModel = true.

@EnergyArchmage
Copy link
Copy Markdown
Contributor Author

EnergyArchmage commented Mar 16, 2018

Well there is tension between filling out std 62.1 tables as much as possible when VRP is not used and doing the calculations properly when VRP is used. But I can rework this to still provide people sums and a diversity of 1.0 without marching thru the schedules. I will push up changes shortly.

@mjwitte mjwitte mentioned this pull request Mar 16, 2018
Closed
11 tasks
@Myoldmopar
Copy link
Copy Markdown
Member

Myoldmopar commented Mar 16, 2018

@EnergyArchmage did you expect that last batch of changes to clean up the diffs "completely"? I'm not saying you need to, I was just curious about your intent with that last set of commits after my diff comment. Since there are still diffs.

@mjwitte
Copy link
Copy Markdown
Contributor

mjwitte commented Mar 16, 2018

We expect diffs, right? Because we're not calculating diversity anymore unless VRP is active.

@EnergyArchmage
Copy link
Copy Markdown
Contributor Author

EnergyArchmage commented Mar 16, 2018

Yes, the Ps value and D value are expected to change because the D is not being accurately calculated. These values are not really used for anything if not using VRP but they still have table entries.

@Myoldmopar Myoldmopar merged commit 8c534eb into develop Mar 17, 2018
@Myoldmopar Myoldmopar deleted the 6487-population-diversity-follow-up branch March 17, 2018 02:24
@mjwitte mjwitte mentioned this pull request Feb 25, 2025
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rraustad rraustad rraustad left review comments

Assignees

No one assigned

Labels

DoNotPublish Includes changes that shouldn't be reported in the changelog

Projects

None yet

Milestone

EnergyPlus 8.9.0

Development

Successfully merging this pull request may close these issues.

7 participants

@EnergyArchmage @Myoldmopar @mjwitte @rraustad @nrel-bot @nrel-bot-2 @nrel-bot-3