Fix EPW parsing bug

[mbadams5]

Pull request overview

Fixes #9161

This was either a copy and paste error or just an error when updating parts of the old code to the newer parsing approach.
This wasn't caught previously because the E+ repo does not have any EPW files with fewer values in the columns than the maximum and thus there are no defaults used during parsing.

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

[mjwitte]

This was either a copy and paste error or just an error when updating parts of the old code to the newer parsing approach.

Looking at 84af30e from #8834, apparently readItem could handle the short line but ProcessNumber can't?

-                        if (!readItem(Line, DaysSinceLastSnow)) {

+                        bool error = false;
+                        DaysSinceLastSnow = UtilityRoutines::ProcessNumber(Line, error);
+                        if (error) {

[Myoldmopar]

I will check and wake CI back up, but otherwise this fix looks fine. I think it would be good for us to add some form of test for this change. Maybe just add a new weather file that specifically has this header variation and use it in a normal integration test. Anything to get E+ to read this variation of header would suffice. I'll think about adding that in unless you want me to hold off for any reason, @mbadams5. Thanks for this fix though.

[mbadams5]

This was either a copy and paste error or just an error when updating parts of the old code to the newer parsing approach.

Looking at 84af30e from #8834, apparently readItem could handle the short line but ProcessNumber can't?

-                        if (!readItem(Line, DaysSinceLastSnow)) {

+                        bool error = false;
+                        DaysSinceLastSnow = UtilityRoutines::ProcessNumber(Line, error);
+                        if (error) {

@mjwitte, No the issue stems from the conversion from Line being a string previously to string_view now (in the variable current_line). In the old code, it would call Line.erase(0, Pos + 1);, which would actually delete the characters from Line, however, now we are using current_line.remove_prefix(pos + 1);. Since current_line is a string_view, remove_prefix will increment the internal index within the string_view, but the code in the spots in this PR still used Line instead of current_line. This means those spots used the full line of data instead of the incremented substrings.

[mbadams5]

@Myoldmopar I just added a new integration test with a weather file that shows the issue in current develop and is fixed with this branch.

[Myoldmopar]

As-is, this will result in this file showing up in installer packages. It can be excluded by prefixing an underscore on the file name, or explicitly listing it in the exclude patterns in cmake/Install.cmake.

Looking forward, if we wanted to use the same example file but for multiple weather files, we could add a new argument to the add_simulation_test function that would add a suffix to the ctest entry to make it unique. That's obviously too much for this little PR, but could be done pretty easily.

For now, unless there is a reason not to, I'll plan on adding an underscore to the filename and we'll wrap this up.

Thanks for adding this test @mbadams5

[mbadams5]

@Myoldmopar, sure that is fine to just add the underscore and move on for this PR. I figured it would add it to the installer but I forgot how to avoid that. The additional argument on the add_simulation_test would be good since I tried to do a duplicate test at first.

[Myoldmopar]

Pushed the rename up. I won't wait on the full CI suite to run, but I'll let a few custom check things run before merging.

[Myoldmopar]

That's enough CI for me, this is great. Thanks @mbadams5 !