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

Clean up options_lookup.tsv in github actions #962

Merged
merged 21 commits into from
Jun 26, 2023
Merged

Clean up options_lookup.tsv in github actions #962

merged 21 commits into from
Jun 26, 2023

Conversation

rajeee
Copy link
Contributor

@rajeee rajeee commented Jul 11, 2022 •
edited by afontani
Loading

Pull Request Description

In an effort to clean up resources/options_lookup.tsv, this sets a standard for what the file should look like. The .gitattributes determines the line ending style. Then trailing whitespace (tabs inserted by Excel), and ordering of values are automatically updated using github actions. The hope is that diffs for new features are easier to look at, conflicts are easier to resolve, and characteristics/options are easier to find.

Updates to options_lookup.tsv

  • Normalize line ending for options lookup to CRLF (like the housing characteristics).
  • Remove leading and trailing whitespace
  • Sort based on parameter and option

Checklist

Not all may apply:

  • Tests (and test files) have been updated
  • Documentation has been updated
  • Changelog has been updated
  • openstudio tasks.rb update_measures has been run
  • No unexpected regression test changes on CI (checked comparison artifacts)

muehleisen pushed a commit to muehleisen/resstock that referenced this pull request Jul 19, 2022
@joseph-robertson
Squashed 'resources/hpxml-measures/' changes from 17acfbcfe29..2224e6…
e3e8246 
…cfcf9

2224e6cfcf9 Merge pull request NREL#974 from NREL/buildreshpxml_validation
5690f8cddf6 Merge branch 'master' into buildreshpxml_validation
82aa7c1e1ba Cleanup.
e30122ae250 Merge pull request NREL#977 from NREL/epvalidator_cleanup
3c8e6682ca5 Clean up apply_defaults arg.
f9a982fd307 Remove test.
ea76e6bddb9 Misc cleanup to EPvalidator.xml. Remove more elements that don't need to be included because they are required by the XSD schema.
c05535eb5f0 Minor cleanup.
9b6360253bd Merge branch 'master' of https://github.com/NREL/OpenStudio-HPXML into buildreshpxml_validation
98f7bee5ed5 Merge pull request NREL#975 from NREL/ci_improvements
b30ad295e9a Merge branch 'master' of https://github.com/NREL/OpenStudio-HPXML into ci_improvements
8e71a4a6760 Fix coverage?
5081166736c Need to run bundle
029d399af05 Bugfix.
0f2285c5806 Splits unit tests and workflow tests into parallel CI jobs. Also fixes NREL#962.
c7b50d42ce3 Unrelated cleanup.
3c960583b4a BuildResidentialHPXML measure: Adds an optional argument for whether the HPXML file is validated; defaults to false. Most workflows were validating the HPXML twice (once at the end of BuildResidentialHPXML and once at the beginning of HPXMLtoOpenStudio), which incurs a performance penalty. We now only apply validation in the BuildResidentialHPXML measure in special circumstances (e.g., creating sample files and unit tests).

git-subtree-dir: resources/hpxml-measures
git-subtree-split: 2224e6cfcf913ac7815b8f4688b250ee5c86f924
@afontani afontani changed the title Normalize line endings in options lookup Clean up options_lookup.tsv in github actions Jun 20, 2023
@afontani afontani force-pushed the normalize_opts_lookup branch 6 times, most recently from 3891acc to e73ffc1 Compare June 21, 2023 15:51
@afontani afontani self-assigned this Jun 21, 2023
@afontani
Copy link
Contributor

afontani commented Jun 24, 2023 •
edited
Loading

@joseph-robertson : The functionality seems to be working. I am a little confused why the test/base_results files are showing changes. The comparisons are also showing a scattered set of results between the base and the feature branch. I don't expect there to be changes. I checked out the most recent result files from develop at 259f732 and they were replaced in the following "Latest Results" commit.

Do you have any thoughts? See attached screenshot of the most recent artifact from 259f732.

Screen Shot 2023-06-24 at 8 44 00 AM

@joseph-robertson
Copy link
Contributor

joseph-robertson commented Jun 26, 2023 •
edited
Loading

@joseph-robertson : The functionality seems to be working. I am a little confused why the test/base_results files are showing changes. The comparisons are also showing a scattered set of results between the base and the feature branch. I don't expect there to be changes. I checked out the most recent result files from develop at 259f732 and they were replaced in the following "Latest Results" commit.

Do you have any thoughts? See attached screenshot of the most recent artifact from 259f732.

Screen Shot 2023-06-24 at 8 44 00 AM

@afontani The set of samples has changed. Looks like sampling is a function of the options_lookup. You can pretty easily verify this by doing a .shuffle on the params array in the link above.

@afontani
Copy link
Contributor

@joseph-robertson : The functionality seems to be working. I am a little confused why the test/base_results files are showing changes. The comparisons are also showing a scattered set of results between the base and the feature branch. I don't expect there to be changes. I checked out the most recent result files from develop at 259f732 and they were replaced in the following "Latest Results" commit.
Do you have any thoughts? See attached screenshot of the most recent artifact from 259f732.
Screen Shot 2023-06-24 at 8 44 00 AM

@afontani The set of samples has changed. Looks like sampling is a function of the options_lookup. You can pretty easily verify this by doing a .shuffle on the params array in the link above.

@joseph-robertson : Good catch!

joseph-robertson
joseph-robertson approved these changes Jun 26, 2023
View reviewed changes
Copy link
Contributor

@joseph-robertson joseph-robertson left a comment

Choose a reason for hiding this comment

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

This looks good to me. Bummer we can't confirm simulation results don't change (because of the samples changing), but I don't see anything in this PR that would affect them anyway.

.github/workflows/config.yml
@@ -251,7 +288,7 @@ jobs:

update-results:
runs-on: ubuntu-latest
needs: [integration-tests]
needs: [format-files,integration-tests]
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

.github/workflows/config.yml
@@ -300,6 +343,7 @@ jobs:
cp -r results/upgrades/timeseries/*.csv test/base_results/upgrades/timeseries
git add test/base_results
git add docs
git add resources/options_lookup.tsv
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like the artifact download correctly places the options_lookup.tsv in the resources folder. So that's good - no copy necessary.

@afontani
Copy link
Contributor

This looks good to me. Bummer we can't confirm simulation results don't change (because of the samples changing), but I don't see anything in this PR that would affect them anyway.

Yes. Agreed.

@afontani afontani merged commit dae0bc1 into develop Jun 26, 2023
@afontani afontani deleted the normalize_opts_lookup branch June 26, 2023 21:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@afontani afontani afontani left review comments

@joseph-robertson joseph-robertson joseph-robertson approved these changes

Assignees

@afontani afontani

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rajeee @afontani @joseph-robertson