Fix all test memory leaks and related issues in ssc and tcs by sjanzou · Pull Request #749 · NatLabRockies/ssc

sjanzou · 2022-02-07T11:20:15Z

No description provided.

…yLeak

memory leak fixed for first two pvwattsv8 tests but remains if all pvwattsv8 tests are run

rewrite create_weatherdata_array and free_weatherdata_array

rewrite create_winddata_array and free_winddata_array

…y leaks

…on, fix memory leak in sscapi_test, json_to_ssc_data

…n_to_ssc_data calls with ssc_data_free

…etest causes memory leaks on windows when failures reported

… leaks on Windows 10 CMPvsamv1BatteryIntegration_cmod_pvsamv1.LCOS_test_cashloan"; // pass with many memory leaks windows 10 CMPvsamv1BatteryIntegration_cmod_pvsamv1.ResidentialDCBatteryModelPriceSignalDispatch"; // pass with many memory leaks windows 10

…ng memory leaks in tests

sjanzou · 2022-02-07T11:23:29Z

No memory leaks on macOs 12.0.1 and all tests passing on Github Actions and Windows.
We can discuss running new tests added in Windows debug mode to prevent future leaks.
The Windows (and mac) SEH should no longer occur.

Questions and comments and updated practices are welcomed!

sjanzou · 2022-02-07T11:26:38Z

The jc-community contributor seems to occur from a different computer I was testing and updating on... interesting how they came into existence...

sjanzou · 2022-02-07T11:29:49Z

There are 6 failing tests only on macOS 12.0.1 with the Apple Silicon chip with maybe too tight of tolerance:

[ RUN ] voltage_table_lib_battery_voltage_test.calculateMaxChargeHourly1
/Users/imacuser/Public/Projects/GitHub/NREL/ssc/test/shared_test/lib_battery_voltage_test.cpp:674: Failure
The difference between cap->SOC() and 99.99 is 0.010000000000005116, which exceeds 1e-2, where
cap->SOC() evaluates to 100,
99.99 evaluates to 99.989999999999995, and
1e-2 evaluates to 0.01.
[ FAILED ] voltage_table_lib_battery_voltage_test.calculateMaxChargeHourly1 (0 ms)
[ RUN ] voltage_table_lib_battery_voltage_test.calculateMaxChargeHourly2
/Users/imacuser/Public/Projects/GitHub/NREL/ssc/test/shared_test/lib_battery_voltage_test.cpp:705: Failure
The difference between max_current_calc and -0.998 is 0.0019999999999997797, which exceeds 1e-3, where
max_current_calc evaluates to -0.99999999999999978,
-0.998 evaluates to -0.998, and
1e-3 evaluates to 0.001.
/Users/imacuser/Public/Projects/GitHub/NREL/ssc/test/shared_test/lib_battery_voltage_test.cpp:709: Failure
The difference between cap->SOC() and 99.984 is 0.016000000000005343, which exceeds 1e-3, where
cap->SOC() evaluates to 100,
99.984 evaluates to 99.983999999999995, and
1e-3 evaluates to 0.001.
[ FAILED ] voltage_table_lib_battery_voltage_test.calculateMaxChargeHourly2 (0 ms)
[ RUN ] voltage_table_lib_battery_voltage_test.calculateMaxChargeHourly3
/Users/imacuser/Public/Projects/GitHub/NREL/ssc/test/shared_test/lib_battery_voltage_test.cpp:756: Failure
The difference between cap->SOC() and 99.99 is 0.010000000000005116, which exceeds 1e-2, where
cap->SOC() evaluates to 100,
99.99 evaluates to 99.989999999999995, and
1e-2 evaluates to 0.01.
/Users/imacuser/Public/Projects/GitHub/NREL/ssc/test/shared_test/lib_battery_voltage_test.cpp:757: Failure
The difference between cap->I() * model->battery_voltage() and -4615 is 1.4800619834704776, which exceeds 1, where
cap->I() * model->battery_voltage() evaluates to -4616.4800619834705,
-4615 evaluates to -4615, and
1 evaluates to 1.
[ FAILED ] voltage_table_lib_battery_voltage_test.calculateMaxChargeHourly3 (0 ms)
[ RUN ] voltage_table_lib_battery_voltage_test.calculateMaxChargeSubHourly1
/Users/imacuser/Public/Projects/GitHub/NREL/ssc/test/shared_test/lib_battery_voltage_test.cpp:786: Failure
The difference between cap->SOC() and 99.99 is 0.010000000000005116, which exceeds 1e-2, where
cap->SOC() evaluates to 100,
99.99 evaluates to 99.989999999999995, and
1e-2 evaluates to 0.01.
/Users/imacuser/Public/Projects/GitHub/NREL/ssc/test/shared_test/lib_battery_voltage_test.cpp:787: Failure
The difference between cap->I() * model->battery_voltage() and -5128 is 1.4222910927455814, which exceeds 1, where
cap->I() * model->battery_voltage() evaluates to -5129.4222910927456,
-5128 evaluates to -5128, and
1 evaluates to 1.
[ FAILED ] voltage_table_lib_battery_voltage_test.calculateMaxChargeSubHourly1 (0 ms)
[ RUN ] voltage_table_lib_battery_voltage_test.calculateMaxChargeSubHourly2
/Users/imacuser/Public/Projects/GitHub/NREL/ssc/test/shared_test/lib_battery_voltage_test.cpp:820: Failure
The difference between cap->SOC() and 99.99 is 0.010000000000005116, which exceeds 1e-2, where
cap->SOC() evaluates to 100,
99.99 evaluates to 99.989999999999995, and
1e-2 evaluates to 0.01.
[ FAILED ] voltage_table_lib_battery_voltage_test.calculateMaxChargeSubHourly2 (0 ms)

brtietz · 2022-02-07T17:38:38Z

Ignore my previous comment - I was running the tests on patch and failed to check out this branch.

brtietz · 2022-02-07T20:32:30Z

No SEH exceptions in one run on Windows (much better than usual!)

The test results make it look like increasing the tolerance is a reasonable option. It looks like most could be increased by 10%, while those currently set to a value of "1" should perhaps be "2".

Matthew-Boyd

The fixed memory leaks and other changes in the CSP files look great, thank you very much for doing this! I guess we'll have to make good lasting impressions now.

Note that lib_battery_voltage_test.cpp line 709 had to be increased from 1e-3 to 2e-2 to pass.

sjanzou · 2022-02-10T11:58:49Z

No SEH exceptions in one run on Windows (much better than usual!)

The test results make it look like increasing the tolerance is a reasonable option. It looks like most could be increased by 10%, while those currently set to a value of "1" should perhaps be "2".

Updated in commit 63b343e with noted tolerance update in detailed commit message.

I would like to merge sooner than later so I do not need to keep updating tests that are modified on the patch branch without these changes ;-)

cpaulgilman

FYI -- these changes to not seem to affect simulation run times, test results generated in Windows 10, Intel Core i5-6400 2.7GHz CPU 4 Cores, 32 GB RAM, no SSD hard drive:

sim-time-test.zip

dguittet

Great job!

sjanzou · 2022-02-11T07:21:20Z

FYI -- these changes to not seem to affect simulation run times, test results generated in Windows 10, Intel Core i5-6400 2.7GHz CPU 4 Cores, 32 GB RAM, no SSD hard drive:

sim-time-test.zip

@cpaulgilman, the column titles in the spreadsheet have 'release' and 'patch'... is the 'release' the patch branch of ssc and the 'patch' branch the ssc_719 branch of ssc (this branch)?

I am not sure why ETES Single Owner is now taking 63s longer....

sjanzou · 2022-02-11T07:21:51Z

Great job!

Thank you and thank you for the quick review!

cpaulgilman · 2022-02-11T21:17:52Z

FYI -- these changes to not seem to affect simulation run times, test results generated in Windows 10, Intel Core i5-6400 2.7GHz CPU 4 Cores, 32 GB RAM, no SSD hard drive:
sim-time-test.zip

@cpaulgilman, the column titles in the spreadsheet have 'release' and 'patch'... is the 'release' the patch branch of ssc and the 'patch' branch the ssc_719 branch of ssc (this branch)?

I am not sure why ETES Single Owner is now taking 63s longer....

Yes, patch = ssc_719 and release = SAM 2021.12.02.

sjanzou and others added 30 commits January 10, 2022 01:57

Merge remote-tracking branch 'origin/patch' into TestsMemoryLeak

eec5e13

Testing memory leaks in Tests

22b6ce6

Testing memory leaks in Tests working on ssc #719

1794adc

Merge remote-tracking branch 'origin/TestsMemoryLeak' into TestsMemor…

ef2bf6b

…yLeak

Merge branch 'patch' into TestsMemoryLeak

db75bbb

update test with access violation on Windows

be1c747

Fix memory leak in var_table::clear() on Windows

1066424

Fix exception thrown for non annual tests

7b97572

memory leak fixed for first two pvwattsv8 tests but remains if all pvwattsv8 tests are run

9 of 11 pvwattsv8 tests running and passing without memory leaks

8fe4125

All 11 pvwattsv8 tests running without memory leaks

ace556f

rewrite create_weatherdata_array and free_weatherdata_array

Fix exceptions and memory leaks in pvwatts compute modules and tests

a91d76d

fix two solar water heating tests - no memory leaks on windows 10

4e3aeb2

Fix memory leaks with csp_solver_core_test UsingDataCaseWeatherReader

6d2c66a

Merge remote-tracking branch 'origin/patch' into ssc_719

a535bd3

all 21 pvsamv1 tests pass with no memory leaks on Windows 10

438c577

Fix failing Github Actions builds c++ unique_ptr implementation

529bd86

All 10 CMWindPowerIntegration pass without memory leaks

e3384a7

rewrite create_winddata_array and free_winddata_array

all 6 Turbine_powercurve_cmod_windpower_eqns tests pass without memor…

a5a93a0

…y leaks

Fix memory leak in set_python_path

6a7011a

working through landbosse memory leaks on Windows

9d9a61a

landbose memory leak checks

de7287d

fix sscapi.h double definition of json_to_ssc_data and ssc_data_to_js…

fd626b2

…on, fix memory leak in sscapi_test, json_to_ssc_data

All 4 sscapi_test pass without memory leaks - must clean up after jso…

b3883b9

…n_to_ssc_data calls with ssc_data_free

add some garbage cleanup to cmod_wind_landbosse to avoid memory leaks

c190a67

Merge remote-tracking branch 'origin/patch' into ssc_719

d3a7864

Fix landbosse load_config() memory leaks on Windows - note that googl…

208c204

…etest causes memory leaks on windows when failures reported

Fix util_rate_data memory leak in battstor

dfa9832

Fix battstor issues with util_rate_data and utilityRate members causi…

35f983a

…ng memory leaks in tests

Merge remote-tracking branch 'origin/patch' into ssc_719

2849f14

sjanzou added 2 commits February 7, 2022 03:21

Fix implicit memory leak in GeothermalPlantAnalyzer

758d7ed

Clean up main.cpp with examples and comments

3486de3

sjanzou requested review from Matthew-Boyd, brtietz, cpaulgilman, dguittet, janinefreeman, mjprilliman and tyneises February 7, 2022 11:20

sjanzou linked an issue Feb 7, 2022 that may be closed by this pull request

Memory leaks in ssc/Tests #719

Closed

Matthew-Boyd approved these changes Feb 8, 2022

View reviewed changes

sjanzou added 3 commits February 10, 2022 00:37

Merge branch 'patch' into ssc_719

49b18c5

Fix faulty Github merge of patch into ssc_719

402c057

Update tolerances per comments on #749 to pass on Apple Silicon

63b343e

Note that lib_battery_voltage_test.cpp line 709 had to be increased from 1e-3 to 2e-2 to pass.

tyneises removed their request for review February 10, 2022 15:23

brtietz approved these changes Feb 10, 2022

View reviewed changes

cpaulgilman approved these changes Feb 10, 2022

View reviewed changes

dguittet approved these changes Feb 10, 2022

View reviewed changes

brtietz mentioned this pull request Feb 10, 2022

Confirm net means net #756

Merged

sjanzou merged commit e839883 into patch Feb 11, 2022

sjanzou deleted the ssc_719 branch February 11, 2022 10:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix all test memory leaks and related issues in ssc and tcs#749

Fix all test memory leaks and related issues in ssc and tcs#749
sjanzou merged 54 commits into
patchfrom
ssc_719

sjanzou commented Feb 7, 2022

Uh oh!

sjanzou commented Feb 7, 2022

Uh oh!

sjanzou commented Feb 7, 2022

Uh oh!

sjanzou commented Feb 7, 2022

Uh oh!

brtietz commented Feb 7, 2022

Uh oh!

brtietz commented Feb 7, 2022

Uh oh!

Matthew-Boyd left a comment

Uh oh!

sjanzou commented Feb 10, 2022

Uh oh!

cpaulgilman left a comment

Uh oh!

dguittet left a comment

Uh oh!

sjanzou commented Feb 11, 2022

Uh oh!

sjanzou commented Feb 11, 2022

Uh oh!

cpaulgilman commented Feb 11, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

sjanzou commented Feb 7, 2022

Uh oh!

sjanzou commented Feb 7, 2022

Uh oh!

sjanzou commented Feb 7, 2022

Uh oh!

sjanzou commented Feb 7, 2022

Uh oh!

brtietz commented Feb 7, 2022

Uh oh!

brtietz commented Feb 7, 2022

Uh oh!

Matthew-Boyd left a comment

Choose a reason for hiding this comment

Uh oh!

sjanzou commented Feb 10, 2022

Uh oh!

cpaulgilman left a comment

Choose a reason for hiding this comment

Uh oh!

dguittet left a comment

Choose a reason for hiding this comment

Uh oh!

sjanzou commented Feb 11, 2022

Uh oh!

sjanzou commented Feb 11, 2022

Uh oh!

cpaulgilman commented Feb 11, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants