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

Weather file update #432

Merged
merged 17 commits into from
Apr 6, 2020
Merged

Weather file update #432

merged 17 commits into from
Apr 6, 2020

Conversation

TobiAdekanye
Copy link
Contributor

@TobiAdekanye TobiAdekanye commented Mar 30, 2020

Pull Request Description

Presently, to update the weather year files, we would need to make changes to the Location Weather Filename and Location Weather Year tsvs. While this approach is fine with two weather years i.e. TMY3 and AMY weather files presently, it creates a 'ballooning' effect when one begins to add more weather years as the location weather filename tsv will need the addition of 216 EPWs to its rows and columns for each AMY year. To resolve this issue, we:

  • rename all epw weather filenames such that names are consistent between all years. New location weather filename data can be found here
  • delete location weather filename and location weather year as the names are now consistent between years
  • update options lookup tsv by adding weather filename arguments to the location tsv
  • update options lookup tsv by removing 'location weather filename' and 'location weather year' arguments

Checklist

Not all may apply:

  • Unit tests have been added or updated
  • All rake tasks have been run, and pass
  • Documentation has been modified appropriately
  • Any new options are added to project_testing
  • project_testing runs without any failures
  • No unexpected regression test changes
  • All tests are passing (green) on circleci
  • The changelog has been updated appropriately
  • This branch is up-to-date with master

For more information on how to perform these checklist items, see the documentation's Advanced Tutorial.

@afontani
Copy link
Contributor

@TobiAdekanye @joseph-robertson Is there a more permanent place we can put these weather files other than SharePoint?

@TobiAdekanye can you also put up the TMY3 files and the testing project files in the same directory?

@joseph-robertson
Copy link
Contributor

@TobiAdekanye @joseph-robertson Is there a more permanent place we can put these weather files other than SharePoint?

@TobiAdekanye can you also put up the TMY3 files and the testing project files in the same directory?

The files themselves? Or do you mean the documentation?

@afontani
Copy link
Contributor

@TobiAdekanye @joseph-robertson Is there a more permanent place we can put these weather files other than SharePoint?
@TobiAdekanye can you also put up the TMY3 files and the testing project files in the same directory?

The files themselves? Or do you mean the documentation?

Sorry for the confusion, the files themselves.

@joseph-robertson
Copy link
Contributor

joseph-robertson commented Mar 31, 2020

@TobiAdekanye @joseph-robertson Is there a more permanent place we can put these weather files other than SharePoint?
@TobiAdekanye can you also put up the TMY3 files and the testing project files in the same directory?

The files themselves? Or do you mean the documentation?

Sorry for the confusion, the files themselves.

They're on data.nrel.gov. Is that what you mean? For example, here is where I patched the multifamily_beta PAT project to pull from there: https://github.com/NREL/OpenStudio-BuildStock/blob/v2.2.3/project_multifamily_beta/pat.json#L20203.

@afontani

@afontani
Copy link
Contributor

afontani commented Mar 31, 2020

@TobiAdekanye @joseph-robertson Is there a more permanent place we can put these weather files other than SharePoint?
@TobiAdekanye can you also put up the TMY3 files and the testing project files in the same directory?

The files themselves? Or do you mean the documentation?

Sorry for the confusion, the files themselves.

They're on data.nrel.gov. Is that what you mean? For example, here is where I patched the multifamily_beta PAT project to pull from there: https://github.com/NREL/OpenStudio-BuildStock/blob/v2.2.3/project_multifamily_beta/pat.json#L20203.

@afontani

That is perfect. Thanks for clarifying @joseph-robertson.

@afontani @joseph-robertson I have put up the weather files on box and modified the link above.

Copy link
Contributor

@afontani afontani left a comment

Choose a reason for hiding this comment

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

@TobiAdekanye In general, this looks pretty good. The only issue I have is that it looks like options_lookup was completely recommitted. I would prefer that the changes to the file reflect the actual changes to the file. I think what might have happened is Excel rewrote all the line endings and therefore thought that all the lines had changed.

@TobiAdekanye
Copy link
Contributor Author

TobiAdekanye commented Mar 31, 2020

@afontani I did not recommit the options_lookup tsv, only deleted those rows i.e. location weather filename and location weather year. I am unsure how to fix this problem.

@TobiAdekanye TobiAdekanye merged commit 6646907 into master Apr 6, 2020
@shorowit
Copy link
Contributor

shorowit commented Apr 8, 2020

Were the changes to Insulation Unfinished Attic.tsv intentional? The PR and CHANGELOG updates have no references to it.

@afontani afontani deleted the weather-file-update branch April 8, 2020 15:49
@afontani
Copy link
Contributor

afontani commented Apr 8, 2020

@shorowit These changes were actually made by @joseph-robertson in PR #426. It seems that the changes were made by the "merge conflict fixes" commit (SHA: 3953ce5). It may be an artifact of a Merge commit of trying to update the weather-file-update branch to master, but the commit message was changed.

@TobiAdekanye can you comment on 1) how you updated the weather-file-update branch with master and 2) how you satisfied the merge conflicts?

image

@shorowit
Copy link
Contributor

shorowit commented Apr 8, 2020

That doesn't seem right. @joseph-robertson's PR you linked to shows the Insulation Unfinished Attic.tsv gaining a new dependency (Geometry Building Type RECS), whereas this PR seems to revert that change.

@afontani
Copy link
Contributor

afontani commented Apr 8, 2020

Well, looks like I missed it during my review. I will attempt to fix this.

@afontani
Copy link
Contributor

afontani commented Apr 8, 2020

I confirmed the file was reverted to the state just before the roofing material restructure (SHA: 9c07840).

nmerket added a commit to NREL/buildstockbatch that referenced this pull request Apr 9, 2020
joseph-robertson added a commit that referenced this pull request Jun 30, 2020
828c59c180 Add default value for schedule csv path.
4a883c4077 Merge branch 'master' into build-res-hpxml-v3
75b13545f5 Fix schedules in tv plug loads test.
e0f27a71a2 Merge pull request #435 from NREL/run_log_os_warnings
bda233d24f Move oga require back up.
9b9256843e Merge branch 'master' into build-res-hpxml-v3
b7d6402f1e Update variable names. [ci skip]
56e109f48a Merge pull request #434 from NREL/rename_energyplus_class
5df23d664f Adds OpenStudio warnings to run.log when using run_simulation.rb.
3520679ae4 Renames the new EnergyPlus class to EPlus to avoid conflicts in some workflows.
4f747e47fc Merge pull request #433 from NREL/window_ufactor_warning
79ff991025 Move oga down into run method.
133a1cf0a5 Remove debug statement.
fb9c2b9057 Adds a warning if the window U-factor is above the EnergyPlus max value and reduces it to an allowed value.
1c49fd9749 Merge pull request #432 from NREL/tv_schedule_bugfix
4b456aefe6 Fixes a typo in the tv schedule that caused all tv energy use to essentially occur in a single hour.

git-subtree-dir: resources/hpxml-measures
git-subtree-split: 828c59c180dd9bc82ea7e3cecf12bed3f787e824
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants