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.

Sign up for GitHub

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

Jump to bottom

C++ CLI: Create results.json and handle analysis.json (create objectives.json) #5028

Merged
merged 10 commits into from
Nov 13, 2023
Merged

C++ CLI: Create results.json and handle analysis.json (create objectives.json) #5028

merged 10 commits into from
Nov 13, 2023

Conversation

jmarrec
Copy link
Collaborator

@jmarrec jmarrec commented Nov 13, 2023

Pull request overview

  • Fixes C++ CLI: results.json is not created #5019
  • Create measure_attributes.json and results.json
  • Handle analysis.json being one dir above the osw, and create objectives.json if found
  • TODO: Need to clarify which of the measure_attributes.json, results.json and data_point_out.json are needed and when: they have the SAME info!

Pull Request Author

  • Model API Changes / Additions
  • Any new or modified fields have been implemented in the EnergyPlus ForwardTranslator (and ReverseTranslator as appropriate)
  • Model API methods are tested (in src/model/test)
  • EnergyPlus ForwardTranslator Tests (in src/energyplus/Test)
  • If a new object or method, added a test in NREL/OpenStudio-resources: Add Link
  • If needed, added VersionTranslation rules for the objects (src/osversion/VersionTranslator.cpp)
  • Verified that C# bindings built fine on Windows, partial classes used as needed, etc.
  • All new and existing tests passes
  • If methods have been deprecated, update rest of code to use the new methods

Labels:

  • If change to an IDD file, add the label IDDChange
  • If breaking existing API, add the label APIChange
  • If deemed ready, add label Pull Request - Ready for CI so that CI builds your PR

Review Checklist

This will not be exhaustively relevant to every PR.

  • Perform a Code Review on GitHub
  • Code Style, strip trailing whitespace, etc.
  • All related changes have been implemented: model changes, model tests, FT changes, FT tests, VersionTranslation, OS App
  • Labeling is ok
  • 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

@jmarrec
WIP For #5019
6d2d0c6
@jmarrec
Port rename_hash_keys from workflow gem util/post_process::run_extrac…
7d528b1 
…t_inputs_and_outputs

Add some testing. Too bad the original ruby implementation has zero tests, it would have save me the trouble of creating it.
@jmarrec
Start a pytest for running classic / labs in an analysis.json case
8e6df5a
@jmarrec
Use a measure that doesn't need a model/sql so testing goes much fast…
d34d137 
…er with --postprocess_only (and it tests that flag too!)
@jmarrec
Implement spitting out results.json and objectives.json when analysis…
3fae861 
….json is present one dir above osw
@jmarrec
Add a script + a convenience target for updating the **build_dir** se…
6174aee 
…b.osm: it's at 1.11.5 so it takes a long time

build/resources/Examples/compact_osw/files/seb.osm
@jmarrec
Making a test fail on purpose with TODO: Need to clarify which of the…
44c2236 
… measure_attributes.json, results.json and data_point_out.json are needed and when: they have the SAME info!
@jmarrec jmarrec added component - Measures component - CLI Pull Request - Ready for CI This pull request if finalized and is ready for continuous integration verification prior to merge. labs labels Nov 13, 2023
@jmarrec jmarrec self-assigned this Nov 13, 2023
jmarrec
jmarrec commented Nov 13, 2023
View reviewed changes
src/cli/test/test_with_analysis.py Outdated
Comment on lines 77 to 85
# TODO: I'm letting this test fail so it's obvious this needs to be addressed
if True: # not is_labs:
# We get the SAME exact info in measure_attributes.json, results.json and data_point_out.json...
# measure_attributes.json is flushed after each apply measure Step (ModelMeasures, EnergyPlusMeasures,
# ReportingMeasures), then at the end of ReportingMeasures it's done once again and results.json is spat out too
# Do we really need the data_point_out.json in addition to this?
# Seems like we could just run the output of results.json/data_point_out.json at the end of the workflow run
# instead
expected_files_in_run_dir.add("data_point_out.json")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@kbenne @brianlball Here is the TODO I want to adress (with a voluntarily failing tests so it doesn't slip by).

Do we really need all three of these json files? if so, when?

@jmarrec jmarrec mentioned this pull request Nov 13, 2023
Open
@jmarrec
Add data_point_out.json as requested
5193679 
I talked to some folks who suggested just adding it, and I filed #5029 for investigating at a later date if we can just use a single file.
@jmarrec jmarrec requested a review from kbenne November 13, 2023 20:59
@ci-commercialbuildings
Copy link
Collaborator

ci-commercialbuildings commented Nov 13, 2023 •
edited
Loading

CI Results for 5193679:

@jmarrec jmarrec merged commit 3f35a4c into develop Nov 13, 2023
2 checks passed
@jmarrec jmarrec deleted the 5019-result_json branch November 13, 2023 22:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kbenne kbenne Awaiting requested review from kbenne

Assignees

@jmarrec jmarrec

Labels
component - CLI component - Measures labs Pull Request - Ready for CI This pull request if finalized and is ready for continuous integration verification prior to merge.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

C++ CLI: results.json is not created
2 participants
@jmarrec @ci-commercialbuildings