Speeding up reduce - #178
Open
martinjohndyer wants to merge 11 commits into
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This has been going for ages, finally time to get it done before going through again to remove PGPLOT.
This branch includes moving some of the key profile fitting functions (which was found to take ~66% of the time when running reduce according to #142) into C++ code, specifically using pybind11 over Cython. At the same time I also moved the existing "support" code over to pybind11, which is basically the single
avgstdfunction used incombine.The old versions of the functions are still kept in
reduce.pyfor comparison and fallback, I've also added a simple test script to compare and time the old and new versions. The output tests all pass, and on my laptop I get a variable but consistent speedup e.g.Doing some overall timing tests on
reduce, with the current master branch:And after these changes:
Now my laptop is clearly slower than @StuartLittlefair's Mac that was getting 130Hz in #142 (comment) before any optimisations, but I'm hoping the level of speed up is maintained.
I didn't go for the idea of a quick reduce preset in genred that we talked about, although I still think that's something worth considering. I did look at adding in extra options like the least squares tolerances, but right now adding extra parameters to the reduce file means old files fail to be read. I think that could be improved, but that'll have to be touched on when PGPLOT is removed so I'll save it until then.
And a note for completeness, I'm not really a C++ coder so translating the existing functions over and looking for optimisations was very much AI-assisted. At least with the test scripts the outputs can be confirmed, but there may be some oddities in there. Based on the lcurve experience though maybe in the end we'd rather pivot to Rust anyway...