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

JSON input format and InputProcessor refactor #5893

Merged
merged 381 commits into from
Feb 21, 2018

Conversation

mbadams5
Copy link
Contributor

This introduces JSON as the new preferred input format. EnergyPlus will still accept IDF input for a TBD number of future releases. Developers will add and change the Energy+.idd until IDF is no longer accepted for input. There is a python script that is built into the build process that converts the IDD to JDD automatically.

To reduce the amount of code changes needed, all InputProcessor functions maintain their signatures and return the same data format, however, they parse and query against the JSON data structure internally. Through our refactoring efforts, we found two functions in InputProcessor that causes O( n^2 ) complexity and were able to change it to O( n ), due to amortized O(1) lookup with unordered_map. The table below shows this speed up for two different files; one reference building and one worst case input with an extreme number of surfaces (45,382 in 80 zones vs 871 in 118 zones). The table also shows the speed up in ProcessInput, parsing input, and parsing schema.

Outpatient prj10
E+ 8.5 Release PR - IDF PR - JSON E+ 8.5 Release PR - IDF PR - JSON
ProcessInput 366 300 (18%) 234 (36%) 4322 3355 (22%) 1637 (62%)
GetSurfaceData 28 13 (54%) 13 (54%) 72688 21001 (71%) 21001 (71%)
GetObjectItem 38 29 (24%) 29 (24%) 41617 333 (99.2%) 333 (99.2%)
VerifyName 2 0 (100%) 0 (100%) 11055 5 (99.9%) 5 (99.9%)
Parse IDF 174 135 (22%) 69 (60%) 4130 3190 (23%) 1472 (64%)
Parse IDD 192 165 (14%) 165 (14%) 192 165 (14%) 165 (14%)

We are using the JSON for Modern C++ library to parse and our own custom JSON Schema validator for validation.

*** NOTE ***

This requires the update to MSVC 2015 so we need to move CI machines and windows developers to Visual Studio 2015. This should be done anyways since MSVC 2015 is C++11 feature complete.

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

Ksenia Burova and others added 30 commits August 24, 2016 19:15
1 test passes, other still fail. I need more information on knowing how
to fix the rest
…_idf, and fixed assert_false to assert_true in 2 unit tests

Also changed expect_true(ErrorsFound) for expect_false, cause we have
warning not an error now
…s. Remove CheckCachedIPErrors function that checked a file that was unused, as well as commenting out other read / write checks on that file.
…ctor

Update input_processor_refactor with the newest develop
…RIC/EnergyPlus into input_processor_refactor

Merge newest develop into input_processor_refactor
…d be more consistent. Reduce the total amount of map lookups as much as possible in the first half of GetObjectItem (alpha field and extension field iteration). This halfway refactored function is running faster in the profiler already
…at in one out of 4 cases if we don't do it in all 4
…sor_refactor"

This reverts commit 8025489, reversing
changes made to c55cbdd.
Testing to see performance impact since names are inherently unique now
due to JSON keys
…hange the way add_missing_field_value works in the interest of increased efficiency
… needed since the addition of the non-special cased Version Energy+ object
This reduces the algorithmic complexity of GetFooInput (really
GetObjectItem) from O( n ^ 2 ) to O( n log n ). This has a dramatic
effect on runtime for GetObjectItem function. We initially thought
incrementing an iterator would be constant time operation but since
this is a map, it is actually linear. This commit changes it to log
time lookup.
…f std::pair and json::const_iterator copies, from input_processor_refactor_performance branch
@Myoldmopar
Copy link
Member

Verification about reverting success: https://gist.github.com/Myoldmopar/aaace5182721a1126654f620c53f55a7

@Myoldmopar
Copy link
Member

Yeah! 1 Test has big ESO diffs! I'm looking at those diffs now.

@Myoldmopar
Copy link
Member

The LshapedZone example file that is showing ESO diffs is shown here compared to develop.

image

It isn't a big diff, but it is big enough to throw warnings.

@mjwitte
Copy link
Contributor

mjwitte commented Feb 20, 2018

@Myoldmopar Are the Std 62 changes present in both? I merged that into develop in the middle of all this.

@Myoldmopar
Copy link
Member

I was just discussing that with @mbadams5. That could definitely be a part of it, but I'm not sure I had pulled the 62.1 into my local develop yet. He's going to be pushing up potentially "final" changes after he merges develop and resolves conflicts. I'll pull immediately and compare to the latest develop at that time.

@mjwitte
Copy link
Contributor

mjwitte commented Feb 20, 2018

@mbadams5 There may be one more merge going into develop,shortly #6454 @Myoldmopar will decide.

@Myoldmopar
Copy link
Member

Good point @mjwitte. As soon as @nrel-bot-3 gets done with its build of 6454 and it looks good, I'll merge it in. That will shut the door to everything except JSON.

@mjwitte
Copy link
Contributor

mjwitte commented Feb 20, 2018

@mbadams5 Is \obsolete supported here? Some files have one less warning now

-   ** Warning ** IP: IDF line~527 Obsolete object=AirTerminal:SingleDuct:Uncontrolled, encountered.  Should be replaced with new object=AirTerminal:SingleDuct:ConstantVolume:NoReheat

@Myoldmopar
Copy link
Member

I verified locally that single remaining ESO diff is now gone.

@Myoldmopar
Copy link
Member

Good to go! Merging!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
IDDChange Code changes impact the IDD file (cannot be merged after IO freeze) NewFeature Includes code to add a new feature to EnergyPlus Performance Includes code changes that are directed at improving the runtime performance of EnergyPlus Refactoring Includes code changes that don't change the functionality of the program, just perform refactoring
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants