Added Field to Site:Location to Allow Weather File Location Data to be Overwritten by User Info

[RKStrand]

Pull request overview

• Fixes Site:Location should not be overridden by weather file #10579
• TheEnergyPlus input object Site:Location has always defined the information for latitude, longitude, time zone, and elevation for a building. However, when a weather file is present and an annual run is being performed, the assumption has always been that the weather file information for these site specific properties would override the values input by the user in the Site:Location object. Users have pointed out that there are certain situations, e.g., when the closest weather station is at a significantly different elevation or in a different time zone, that it would be helpful to NOT use the site related information from the weather file rather than what was entered by the user. Thus, a new feature was added that allows the user to decide wether or not to use the information from the Site:Location information in the IDF or still override that information when a weather file is present. The default for this is to continue what E+ has done historically. The new input field is added at the end of the Site:Location object and is optional. Thus, no transition is needed since the default for the new field maintains the status quo. This pull request includes a unit test to show that the new feature is working properly and modifications/additions to the Input Output Reference document. The only differences in the results are the there is a new field with a default which causes some minor table adjustments (number of defaults). No changes in the actual output of E+ were noted as expected.

NOTE: ENHANCEMENTS MUST FOLLOW A SUBMISSION PROCESS INCLUDING A FEATURE PROPOSAL AND DESIGN DOCUMENT PRIOR TO SUBMITTING CODE

Pull Request Author

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)
• Label the PR with at least one of: Defect, Refactoring, NewFeature, Performance, and/or DoNoPublish
• Pull requests that impact EnergyPlus code must also include unit tests to cover enhancement or defect repair
• Author should provide a "walkthrough" of relevant code changes using a GitHub code review comment process
• If any diffs are expected, author must demonstrate they are justified using plots and descriptions
• If changes fix a defect, the fix should be demonstrated in plots and descriptions
• If any defect files are updated to a more recent version, upload new versions here or on DevSupport
• If IDD requires transition, transition source, rules, ExpandObjects, and IDFs must be updated, and add IDDChange label
• If structural output changes, add to output rules file and add OutputChange label
• If adding/removing any LaTeX docs or figures, update that document's CMakeLists file dependencies

Reviewer

This will not be exhaustively relevant to every PR.

• Perform a Code Review on GitHub
• If branch is behind develop, merge develop and build locally to check for side effects of the merge
• 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
• Check that performance is not impacted (CI Linux results include performance check)
• Run Unit Test(s) locally
• Check any new function arguments for performance impacts
• Verify IDF naming conventions and styles, memos and notes and defaults
• If new idf included, locally check the err file and other outputs

[rraustad]

Example file name. I'm not really a fan of adding new example files when this feature could be added to an existing example file. Repo size, local install size, time to test regression, etc. are impacted with each new example file. But I do understand the need for specific example files to fail, because of future code changes, for specific reasons.

[RKStrand]

I get that we don't want to just add files without real need. I can always remove this file and simply change an existing file. So, would you prefer that I do that or just correct the example file name, leaving the new example file in there?

[rraustad]

I only have an opinion, the action is yours.

[RKStrand]

Just committed a change that corrects the example file name shown in the comment and also adds an additional comment to explain what this file does.

[Myoldmopar]

I can see both sides here. In this case, since it is a complete change in "how the file runs" or whatever we want to call it, I don't mind it being in a standalone file. If we were, for example, adding a new field to a chiller, we'd probably want to just modify an existing chiller test file.

[rraustad]

If the user said to use the site location, is a message really needed to verify that choice?

[RKStrand]

Good point. I didn't see this as "required" but thought it was simply a helpful "confirmation". Do you want me to take this out? I don't have a problem removing it.

[rraustad]

I think I would rather not see a confirmation of an input. I also don't see a change to the eio. Not sure if that's needed but ...

! <Site:Location>, Location Name, Latitude {N+/S- Deg}, Longitude {E+/W- Deg},  Time Zone Number {GMT+/-}, Elevation {m},  Standard Pressure at Elevation {Pa}, Standard RhoAir at Elevation

[RKStrand]

In the commit that was just made, I got rid of this confirmation message. As far as the EIO is concerned, it's going to show the correct location title based on the new field in Site:Location. That won't show any changes in IDFs that already existed because these all use the default of "No" which means no change from what was done before. If you change any existing input file to "Yes" you will see a change in the EIO. So, I don't think there needs to be any change to the code that produces the EIO unless I am missing something or misunderstood your point.

[Myoldmopar]

Yeah a default of No, to ensure unchanged behavior by default, is good.

[RKStrand]

A note to potential reviewers. With the latest commit entitled Correct Logic Error in #10579 (ef51925), the differences in tables seem to all relate to the fact that a new input field added a new default value. To me, that is expected because the IDD has changed. So, while there are lots of changes, I think they are "expected" and correct.

[Myoldmopar]

So, while there are lots of changes, I think they are "expected" and correct.

Sure, that makes sense, they look like just a bunch of expected table diffs.

[Myoldmopar]

From a code only perspective, this seems pretty reasonable. A new optional field on an existing object, some relevant code changes, with accompanying doc updates, a new example file, and a nice unit test. Nothing further inside here. The real bulk of reviewing this is about the impact and side effects of such a change.

[Myoldmopar]

Since this is not changing things by default, I'm good with it going in as is. I think some users will like turning this on and using it, many will not use it. Let's merge this, thanks @RKStrand and @rraustad