Finish removing Objexx gio #8248

lefticus · 2020-09-02T23:33:09Z

Pull request overview

Finishes removing Objexx gio and support libraries from Energy+

No expected change in tests.

…ving_gio

Myoldmopar · 2020-09-29T16:16:17Z

Pulled develop in one more time to get fresh CI results. I'm building locally then I'll push that up.

Myoldmopar

Looks like lots of great cleanup here. I'll be leaning on CI pretty heavily for verifying the outputs, but I'll definitely do some testing locally as well.

…agedTerminalHeatPump integration test

mitchute · 2020-09-30T16:20:06Z

@lefticus @Myoldmopar running integration tests locally on my Mac showed that libenergyplus.dylib was having troubles loading CurveManagerData.

$ ctest -V -R EMSCurveOverride_PackagedTerminalHeatPump
UpdateCTestConfiguration  from :/Users/mmitchel/Projects/EnergyPlus/dev/EnergyPlus/cmake-build-release/DartConfiguration.tcl
Parse Config file:/Users/mmitchel/Projects/EnergyPlus/dev/EnergyPlus/cmake-build-release/DartConfiguration.tcl
UpdateCTestConfiguration  from :/Users/mmitchel/Projects/EnergyPlus/dev/EnergyPlus/cmake-build-release/DartConfiguration.tcl
Parse Config file:/Users/mmitchel/Projects/EnergyPlus/dev/EnergyPlus/cmake-build-release/DartConfiguration.tcl
Test project /Users/mmitchel/Projects/EnergyPlus/dev/EnergyPlus/cmake-build-release
Constructing a list of tests
Done constructing a list of tests
Updating test list for fixtures
Added 0 tests to meet fixture requirements
Checking test dependency graph...
Checking test dependency graph end
test 260
    Start 260: integration.EMSCurveOverride_PackagedTerminalHeatPump
260: Test command: /Applications/CLion.app/Contents/bin/cmake/mac/bin/cmake "-DSOURCE_DIR=/Users/mmitchel/Projects/EnergyPlus/dev/EnergyPlus" "-DBINARY_DIR=/Users/mmitchel/Projects/EnergyPlus/dev/EnergyPlus/cmake-build-release" "-DENERGYPLUS_EXE=/Users/mmitchel/Projects/EnergyPlus/dev/EnergyPlus/cmake-build-release/Products/energyplus-9.4.0" "-DIDF_FILE=EMSCurveOverride_PackagedTerminalHeatPump.idf" "-DEPW_FILE=USA_IL_Chicago-OHare.Intl.AP.725300_TMY3.epw" "-DENERGYPLUS_FLAGS= -D -r" "-DBUILD_FORTRAN=ON" "-DTEST_FILE_FOLDER=testfiles" "-DRUN_CALLGRIND:BOOL=FALSE" "-DVALGRIND=" "-P" "/Users/mmitchel/Projects/EnergyPlus/dev/EnergyPlus/cmake/RunSimulation.cmake"
260: Test timeout computed to be: 1500
260: dyld: Symbol not found: __ZTVN10EnergyPlus16CurveManagerDataE
260:   Referenced from: /Users/mmitchel/Projects/EnergyPlus/dev/EnergyPlus/cmake-build-release/Products/energyplus-9.4.0
260:   Expected in: /Users/mmitchel/Projects/EnergyPlus/dev/EnergyPlus/cmake-build-release/Products/libenergyplusapi.9.4.0.dylib
260:  in /Users/mmitchel/Projects/EnergyPlus/dev/EnergyPlus/cmake-build-release/Products/energyplus-9.4.0
260: Test Failed
1/1 Test #260: integration.EMSCurveOverride_PackagedTerminalHeatPump ...***Failed  Error regular expression found in output. Regex=[Test Failed]  0.07 sec
0% tests passed, 1 tests failed out of 1
Total Test time (real) =   0.26 sec
The following tests FAILED:
        260 - integration.EMSCurveOverride_PackagedTerminalHeatPump (Failed)
Errors while running CTest

The clear_state on that got left in an experimental state from #8318, so once that was cleaned up the above problems went away. However, after that E+ errored out due to not being able to parse the D0 characters in the EMS objects.

Program Version,EnergyPlus, Version 9.4.0-aea78e99d6, YMD=2020.09.30 09:46,
   ************* Beginning Zone Sizing Calculations
   ** Warning ** Weather file location will be used rather than entered (IDF) Location object.
   **   ~~~   ** ..Location object=MIAMI INTL AP FL USA WMO=722020
   **   ~~~   ** ..Weather File Location=Chicago Ohare Intl Ap IL USA TMY3 WMO#=725300
   **   ~~~   ** ..due to location differences, Latitude difference=[16.16] degrees, Longitude difference=[7.62] degrees.
   **   ~~~   ** ..Time Zone difference=[1.0] hour(s), Elevation difference=[1727.27] percent, [190.00] meters.
   ** Severe  ** EMS Parse Expression, for "CURVEOVERWRITEMGR".
   **   ~~~   ** ...Line="SET C1 = 0.942567793D0".
   **   ~~~   ** ...Bad String="0.942567793D0".
   **   ~~~   ** Invalid numeric="0.942567793D0".
   **  Fatal  ** EMS, previous errors cause termination.
   ...Summary of Errors that led to program termination:
   ..... Reference severe error count=1
   ..... Last severe error=EMS Parse Expression, for "CURVEOVERWRITEMGR".

Once those D0 characters were removed, the test passed locally. It's not complaining about the E0 characters, though.

Myoldmopar · 2020-09-30T17:10:42Z

@mitchute thanks for pushing that up. The D0 was also found in one other example file, which is still failing on Mac. It's good to leave it there though as a test case. Something just needs to be tweaked with the string handling in this branch to get D0 to work and then that file should pass.

lefticus · 2020-09-30T17:47:33Z

@Myoldmopar are these inputs that are failing on Mac new, or were they overlooked when I first created this PR?

If you need me to look at this I'm going to be completely busy through Friday, but able to first thing next week.

Myoldmopar · 2020-09-30T17:57:12Z

I did not verify when they were introduced, though I could run an automated bisection to find the exact commit. And this is not urgent. We are past 9.4.0 release, and just getting some of these relatively easy-to-review PRs merged in for the next release.

lefticus · 2020-09-30T18:56:19Z

Sounds good, let me know if there's anything you'd like me to look at.

Myoldmopar · 2020-10-08T17:56:15Z

I haven't been able to diagnose exactly what caused it, but with this branch, if I add a d0 suffix to any floating point input field, it fails. I think maybe we just need to add handling d0 as a suffix. e0 is working fine, so wherever that is handled, just handle d0.

lefticus · 2020-10-08T18:01:25Z

I haven't been able to diagnose exactly what caused it, but with this branch, if I add a d0 suffix to any floating point input field, it fails. I think maybe we just need to add handling d0 as a suffix. e0 is working fine, so wherever that is handled, just handle d0.

e0 is going to be parsed as "10^0" I have no idea what d0 is supposed to mean. So the e0 is accidentally working currently. Can someone point me to the actual example that is failing and what the intended value is?

Myoldmopar · 2020-10-08T18:15:13Z

OK, I understand this a bit more now. In raw numeric input fields it is not allowed, even in the develop branch. The problem shows up in EMS commands.

In EMS, as you likely know, the user writes their own functions in a built-in language, the EnergyPlus Runtime Language. The interpreter for this is built inside EnergyPlus and the user-defined functions are executed at runtime.

In this custom language, the user can assign numeric literals. Multiple suffixes are allowed, including both e and d for scientific notation.. The historical basis is that in Fortran, and maybe other languages, d was used as the scientific notation for "double precision" variables. So when the EnergyPlus Runtime Language was created, d was also allowed as a scientific notation for numeric literals. In our case here it would be exactly equivalent to e.

If you look in commit ddaa942, you will see that @mitchute removed the d0 from some of the EMS assignments. After he made that change, this file ran successfully. In the CI comment above, you will see one file still failing. That file still has EMS commands with d0 being used, and should continue to do so because we can't break that...at least not without justification.

So when EMS is interpreting a user-defined function, there could be a command line like: SET C1 = 0.942567d0. When the EMS interpreter tries to parse out that numeric literal, the d should be treated similar to an e. Apparently the EMS interpreter is using something that this branch has changed because now those are not allowed.

Let me know if you want some more clarification.

lefticus · 2020-10-08T19:26:06Z

This actually makes perfect sense. Since all of the GIO has been removed through the system (including EMS) and replaced with more standard things. I can fix this.

* convert `d` or `D` to `e` when appropriate and pass to normal normal parsing code * try to make failure cases between libc++ (used on MacOS and some Clang installs) and MSVC/libstdc++ the same in the case of failed parses.

… finish_removing_gio

lefticus · 2020-10-09T19:44:57Z

I believe this last commit 19d9855 addresses the parsing problems with d and D while making failures consistent with MacOS

Myoldmopar · 2020-10-18T01:31:52Z

Deleted one set of results above to confirm security issue is still there. If it is, I'll try merging my Decent PR tomorrow and re-running. Either way, this branch should be all ready to go in.

Myoldmopar · 2020-10-19T00:39:24Z

A rerun showed this was still fine, but I need to verify it builds with latest develop. After that passes I'll get this merged in.

Myoldmopar · 2020-10-19T21:14:07Z

Still all really good. I'm going to build locally to make sure that last develop movement didn't cause the build to break then merge.

mitchute · 2020-10-19T21:19:49Z

@Myoldmopar should we revert out these changes I made, or are they covered elsewhere?

https://github.com/NREL/EnergyPlus/pull/8248/files#diff-3dfc412d9315f968fbb90b031d56b9b077017c5ba5cd549d061648deb820ad6bR145-R146

Myoldmopar · 2020-10-19T21:28:41Z

I say leave them. They are still present in the other test file so we are still covered there.

Myoldmopar · 2020-10-19T21:45:25Z

Built fine locally with latest develop, calling this one done. Thanks @lefticus and @mitchute !

lefticus added 10 commits August 30, 2020 17:12

Remove all unused references to gio

26800f6

Move HeatBalanceManager from gio::read

33a889b

Second round of removing gio for string parsing

0187fc8

Refactor reading / parsing code

0474df9

Add unit tests for new string parsing utilities

648af8b

Remove Objexx::gio

35d209d

Merge branch 'develop' of github.com:NREL/EnergyPlus into finish_remo…

51f5588

…ving_gio

Remove remaining file IO files from objexx

38f24aa

Remove unused char.constants

8b26e0d

Warning fix, CMake formatting fix

335d201

lefticus requested a review from Myoldmopar September 2, 2020 23:33

Myoldmopar added the Refactoring Includes code changes that don't change the functionality of the program, just perform refactoring label Sep 3, 2020

Myoldmopar added this to the EnergyPlus Future milestone Sep 3, 2020

Merge develop; resolve known conflicts

b291f36

Myoldmopar approved these changes Sep 29, 2020

View reviewed changes

mitchute added 2 commits September 30, 2020 08:56

Merge branch 'develop' into finish_removing_gio

aea78e9

fix CurveManagerData clear_state; remove d0 for EMSCurveOverride_Pack…

ddaa942

…agedTerminalHeatPump integration test

lefticus added 2 commits October 9, 2020 13:32

Fix parsing of floating point using d or D for exponent

19d9855

* convert `d` or `D` to `e` when appropriate and pass to normal normal parsing code * try to make failure cases between libc++ (used on MacOS and some Clang installs) and MSVC/libstdc++ the same in the case of failed parses.

Merge branch 'finish_removing_gio' of github.com:NREL/EnergyPlus into…

a8948e5

… finish_removing_gio

Merge remote-tracking branch 'origin/develop' into finish_removing_gio

4e75f65

Myoldmopar merged commit e8b9b22 into develop Oct 19, 2020

Myoldmopar deleted the finish_removing_gio branch October 19, 2020 21:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Finish removing Objexx gio #8248

Finish removing Objexx gio #8248

lefticus commented Sep 2, 2020

Myoldmopar commented Sep 29, 2020 •

edited

Loading

Myoldmopar left a comment

mitchute commented Sep 30, 2020

Myoldmopar commented Sep 30, 2020

lefticus commented Sep 30, 2020

Myoldmopar commented Sep 30, 2020

lefticus commented Sep 30, 2020

Myoldmopar commented Oct 8, 2020

lefticus commented Oct 8, 2020

Myoldmopar commented Oct 8, 2020 •

edited

Loading

lefticus commented Oct 8, 2020

lefticus commented Oct 9, 2020

Myoldmopar commented Oct 18, 2020

Myoldmopar commented Oct 19, 2020

Myoldmopar commented Oct 19, 2020

mitchute commented Oct 19, 2020

Myoldmopar commented Oct 19, 2020

Myoldmopar commented Oct 19, 2020

Finish removing Objexx gio #8248

Finish removing Objexx gio #8248

Conversation

lefticus commented Sep 2, 2020

Pull request overview

Myoldmopar commented Sep 29, 2020 • edited Loading

Myoldmopar left a comment

Choose a reason for hiding this comment

mitchute commented Sep 30, 2020

Myoldmopar commented Sep 30, 2020

lefticus commented Sep 30, 2020

Myoldmopar commented Sep 30, 2020

lefticus commented Sep 30, 2020

Myoldmopar commented Oct 8, 2020

lefticus commented Oct 8, 2020

Myoldmopar commented Oct 8, 2020 • edited Loading

lefticus commented Oct 8, 2020

lefticus commented Oct 9, 2020

Myoldmopar commented Oct 18, 2020

Myoldmopar commented Oct 19, 2020

Myoldmopar commented Oct 19, 2020

mitchute commented Oct 19, 2020

Myoldmopar commented Oct 19, 2020

Myoldmopar commented Oct 19, 2020

Myoldmopar commented Sep 29, 2020 •

edited

Loading

Myoldmopar commented Oct 8, 2020 •

edited

Loading