Refactor evolve.py to wrap new C++ routines#343
Draft
mabruzzo wants to merge 7 commits into
Draft
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.
TODO: The main thing that needs to be done is to convert
evolve_freefall(this shouldn't involve too much effort -- I already converted chunks of the logic)Overview
Essentially I have replaced
evolve_constant_densitywith a Cython function that now wraps some C++ code. This significantly speeds up the freefall and cooling_cell test problems. The speedup is most notable for the freefall problems.Motivation
I started doing this in order to speed up the pygrackle test suite. The impetus is PR #336, which adds so many tests that running the answer-tests takes ~25 minutes.
I was also motivated to write this in a way to make the machinery easily to directly use from C++. This opens the door to reusing this logic in the future for rerunning the Python test-problems when trying to debug memory issues (e.g. with valgrind). We could also potentially write benchmarks for tracking code performance.
Description
TODO: describe how the new code works
This solution is a little over-engineered. I ended up building a whole framework, which makes reuse of this machinery easier (e.g. for when I convert
evolve_freefall). We probably need to re-assess whether this whole framework approach is worthwhile after I convertevolve_freefall.As an aside, I ran into a bunch of unexpected difficulties using Cython. I was a little surprised that there are some areas of C++ where it is very incompatible. It is totally manageable, but it caused me to waste a bunch of time before I fully understood the limitations.
This solution currently makes heavy use of function pointers. It would be worth checking whether the removal of these indirections have a significant impact on performance. Frankly, I'm skeptical that they will change much, but then again, I was a little surprised by just how much time these changes in a single freefall.py test-scenario.
I need to circle back to this later. But, on my machine the answer-tests run in ~25% less time. Interestingly, the changes highlighted in #339 (not implemented here) will probably shave another, unrelated 25% of the the runtime. But, I suspect that the changes of this PR will (and not the ones described in #339), will have a very sizable impact on the runtime of the answer-tests added by #336.