-
Notifications
You must be signed in to change notification settings - Fork 26
Feature 1899 num array #1944
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
Feature 1899 num array #1944
Conversation
…r<double> e. Modifying functions accordingly. SL
…tions instead of arrays. In progress. SL
…es to vector. SL
…Array::clear() instead of vector::clear().
…ging. In progress. SL
… In progress. SL
…. In progres. SL
…vector for storage (instead of array). In progress. SL
…tc_poly_array_alloc_inc instead of num_array_alloc_inc in tc_poly.cc.
…ning make test. SL
…fter switching to STL::vector, it'll ALWAYS be an exact allocation. Previously, we always rounded up to the next allocation increment, but there's no allocation increment anymore. Updating the other code to removes calls to exact.
…nd vector.reserve(). SL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I approve of these changes.
Seth and I discussed them extensively and I reviewed the final code changes. I also ran a full regression test vs the develop branch and it flagged no diffs in:
kiowa:/d1/projects/MET/MET_pull_requests/met-10.1.0/met-10.1.0_beta4/pr1944_feature_1899/test_regression.log
Running an adapted version of regression_runtimes.ksh, I see that most runtimes are very similar, a couple have sped up, and a few have increased.
./regression_runtimes.ksh test_unit_develop.log test_unit_feature_1899_num_array.log | sort -n +8 > runtime_diffs.txt
I manually re-ran the test for the largest increase, and do confirm a similar increase in runtime:
develop = 66.427 feature_1899_num_array = 82.721 DIFF = 16.294 TEST = climatology_POINT_STAT_GFS_CLIMO_PREV_MONTH
Here's the output of "/usr/bin/time -v" for:
develop:
User time (seconds): 67.01
Maximum resident set size (kbytes): 220208
feature_1899_num_array:
User time (seconds): 81.96
Maximum resident set size (kbytes): 220228
I certainly wish we were NOT increasing the runtime. I did do some testing but could not find a simple solution.
Pull Request Testing
Extensive stress testing was done to gauge memory reduction and runtime implications for this change. Most of the testing is described in the issue:
#1899
Run single or large stress tests as described here (the input data for the stress test is provided in the issue tracker):
#1899
Yes. No functionality was changed so no documentation needs to be updated.
See:
#1899
If yes, describe the new output and/or changes to the existing output:
No
Pull Request Checklist
See the METplus Workflow for details.
Select: Reviewer(s)
Select: Organization level software support Project or Repository level development cycle Project
Select: Milestone as the version that will include these changes