Add performance test #275
Add performance test #275
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #275 +/- ##
==========================================
+ Coverage 96.50% 96.61% +0.10%
==========================================
Files 16 17 +1
Lines 1919 1947 +28
==========================================
+ Hits 1852 1881 +29
+ Misses 67 66 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
For the raw benchmark threshold I set that However already this threshold was too stringent for the github action runners. My question: how should we put in this benchmark? Should I just increase the factor to 8? Do something else? I'm not sure a good way to test for performance regression since absolute timing is so system dependent. |
|
there's libraries built spefically for this purpose rather than writing your own tests like that. pint uses codspeed |
|
Perfect, that's exactly the sort of thing I will look for. I will try to integrate codspeed. |
|
I made a complexity only test which runs the benchmark for n = [10, 100, ...] and checks that log10(t/t0) is equal to log10(n/n0) to within 10%. Looking at t vs n on a log plot the data is very linear, so I expect this test to be relatively robust. It should definitely catch regressions if t becomes proportional to n^2 instead of n. The worry is that it will occasionally fail erroneously (false negative). |
|
I'm hitting a variety of issues with establishing the connection to codspeed.io, but I'll keep working at it. |
|
@jagerber48 @andrewgsavage Thanks for this. I approved the codspeed integration request. I also agree with the observation in #274 that the lazy evaluation from @lebigot is very impressive and nicely avoids calculations of uncertainties on intermediate quantities that are never needed in isolation. Clever! And, yes, I can see that this would be easy to slip up when refactoring. So, thanks, and sorry I can't be more help at this time. |
|
@newville thank you for approving the integration request. However I see that further configuration is needed. I got a message that the configuration can only be done by an organization owner (which I am not). This is the configuration that I think needs to be done I access it from the repositories page on my codecov.io account page, then I click to import a new repository (+) and then configure the lmfit organization. Looks like within the I've never used codspeed, I don't know if further "owner-only" configuration will be necessary after this. |
|
Ok, it looks like I am past the issue from my last comment. Not sure if that is due to someone else doing something, or if it just started working. Either way, the ball is in my court now to setup the CI to push some runs of the benchmarking up to codspeed. |
CodSpeed Performance ReportCongrats! CodSpeed is installed 🎉
You will start to see performance impacts in the reports once the benchmarks are run from your default branch.
|
| assert 0.9 * log10(n / n0) < log10(t / t0) < 1.1 * log10(n / n0) | ||
|
|
||
|
|
||
| @pytest.mark.benchmark |
There was a problem hiding this comment.
you can probably parameterise this test with n_list and have it benchmark each num
There was a problem hiding this comment.
Nesting the benchmark and parametrize marks works in my local environment, but seems to make the github action hang forever running the test.
There was a problem hiding this comment.
Ok got it. The problem is I was mixing my internal benchmarking using timeit with what codspeed was trying to do. The tool is very nice.
Will cleanup the tests a little bit now that I know what's going on then get this ready for review.
…ture/benchmark_test
|
This is ready for review. @andrewgsavage you seem like the natural person for this one. |
| return result | ||
|
|
||
|
|
||
| def test_repeated_summation_complexity(): |
There was a problem hiding this comment.
I've seen a test similar to this that kept randomly failing so I don't know how reliable this test will be.
I think test_repeated_summation_speed will catch any increases in time so these tests are testing the same thing. I'm not opposed to this test- we can leave it in for now, and if it we find it unreliable we can remove it.
I'll add a link to PR with the graph you plotted as it helps makes sense of this test. It'll be good to see this plotted for your other PR too.
pre-commit run --all-fileswith no errorsadd a performance benchmark test. This test is important especially to ensure #262 doesn't introduce a performance regression.