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

Use local nodes for air flow network calculation #6263

Closed
wants to merge 54 commits into from

Conversation

xuanluo113
Copy link
Contributor

Pull request overview

  1. Use local nodes for air flow network calculation
  2. Tests and docs for local environmental data for urban context

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

lefticus and others added 30 commits December 3, 2014 11:50
Spacing only change to clean up diffs
Remove {Basedon . . . } endline tags
…used in GetInput.

EvaporativeCoolers correction fixed issue, ChillerElectricEIR was found during diagnosis.
Example file now shows increased water use rate since drift and blowdown are now being used correctly.
This should clear up reports of IdfEditor and EPLaunch failing on Windows
8 and perhaps other systems.

[#81032236]
This fixed the NaN issue on my Linux machine, will have to try on Windows and see;
Perhaps this will fix #4529;
[#71605704]
Seems it is nearly a bug spread between g++ and gtest.  Gtest does some template magic that is OK by C++ standard, but could be modified to work better.  In any case, for these true/false cases, just go with the EXPECT_TRUE and EXPECT_FALSE functions instead of EXPECT_EQ( true, ... ) and the warning wont show up;
http://code.google.com/p/googletest/issues/detail?id=322
[#82514990]
The data for DaylightMaps was not getting inserted into the SQLite
database due to an incorrect variable used in the preparedStatement.
During conversion from Fortran to C++ a bug was introduced causing the row and column
labels of tabular reports in sqlite db to be mixed up.  The origins
having to do with FArray being column major and the offending sqlite
output code using the linearized index.

[#83704438]
Fixes #4602
This file is used by EP-Launch to notify about updates.  The old url
would not render properly in a browser because of how GitHub servers
raw.github.com content.  This change points to a github pages page,
which renders html as expected.
SimObjectError = true;
break;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

@jasondegraw I mean this seems OK to me. Inside the SurfaceAverageCalculation block, looping over all nodes and throwing if any of them are local nodes. That's the gist of what error you were wanting, right?

Copy link
Member

Choose a reason for hiding this comment

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

@Myoldmopar I guess, but it still doesn't look quite right to me. If it looks OK to you, it's probably OK, though, so I'll approve here in a second.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks @jasondegraw, I think the only issue holding this up now is the unit test failure.

@Myoldmopar
Copy link
Member

There is still a unit test failure here. @XuanLuoLBL please address. Once this unit test failure is fixed, CI will be all clean.

@Myoldmopar
Copy link
Member

@XuanLuoLBL you just pulled master into this branch. That's bad. You need to revert that back out. We work off of develop, not master.

@Myoldmopar
Copy link
Member

But the diffs on the PR files changed tab look fine. What happened here?

@Myoldmopar Myoldmopar added the NewFeature Includes code to add a new feature to EnergyPlus label Sep 20, 2017
@xuanluo113
Copy link
Contributor Author

@Myoldmopar Weird that the file changes are rendering the reverted commit. And actually commit "dcd5a01" doesn't show any file change at all.

My branch is up to date to develop right now.

Got trapped by this and I'll never ever ever use that windows git console again...

Are things back to normal now?

@mjwitte
Copy link
Contributor

mjwitte commented Sep 20, 2017

@XuanLuoLBL If I'm following this correctly, you haven't reverted the problem merge from master yet. That was 2169d45 But don't do anything more until @Myoldmopar confirms.

@Myoldmopar
Copy link
Member

Correct; something is still wrong.

@xuanluo113
Copy link
Contributor Author

@mjwitte @Myoldmopar 2169d45 only added a blank space. And dcd5a01 did nothing at all. And that's why it didn't bother me then.
And now when I wish to pull from origin/develop, it shows up-to-date, which is not...

@xuanluo113
Copy link
Contributor Author

@Myoldmopar I actually feel if I revert the reverted commit things gonna be fine. Or shall I checkout some specific commit?

@Myoldmopar
Copy link
Member

I will push up a different branch in a minute and we're closing this PR and opening a new one to merge from.

@xuanluo113
Copy link
Contributor Author

Okay thanks. Sorry about the mess.

@Myoldmopar
Copy link
Member

I still can't get it. I'll have to try again tomorrow.

@xuanluo113
Copy link
Contributor Author

@Myoldmopar Can I just open up another branch and delete this one? I dare not do anything before you confirm now.

The unit test issue is killing me as well. It passes perfectly on my local machine every time from the very beginning and I just don't know what might trigger the difference. Tried lots of time.

@Myoldmopar
Copy link
Member

I just opened #6335 after what may have been a successful last try. If that works, we'll just move forward with that one. If not, then we'll just figure it out tomorrow.

@xuanluo113
Copy link
Contributor Author

@Myoldmopar Thanks.

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet