Skip to content
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

Refactor RK4 integration #8

Open
wants to merge 2 commits into
base: runge-kutta
Choose a base branch
from

Conversation

jacione
Copy link

@jacione jacione commented Jan 2, 2025

The nature of RK4 integration requires a bit of a rethink into how the particle updates are parallelized. I've refactored both the Euler and RK4 updateVelocity functions to place the parallelization inside the scope of the functions themselves. Assuming I've done this correctly, this should now be able to correctly implement RK4, as well as providing additional flexibility if/when you decide to try more integrators.

I spent a few days trying to setup an environment for developing Java, but I couldn't figure out how to get the app to import from my local code. As such, this code is completely untested! I realize that this is a terrible practice, and I apologize for the fact that I'm essentially asking you to debug my code. Having said that, I think I understand at least the organization of your code well enough to provide a decent starting point. A few points to be aware of:

  1. I'm targeting the PR at your runge-kutta branch, so at the very least, it won't ruin everything.
  2. I saw that there was a particlesBuffer attribute of the Physics class which didn't seem to get used anywhere, so I used it to hold onto the temporary steps involved in RK4. If that was actually something important, you should be able to just create another array of particles as a public attribute.
  3. There are 5 calls (not 4) to loadDistributor.distributeLoadEvenly within updateVelocityRK4. The first 4 calculate the k-values, and the 5th applies them.
  4. Each substep in the RK4 process is done using the initial containers. Essentially this bakes in the assumption that rmax is much larger than the distance the particle moves in one step. If this assumption breaks down, I expect it would cause problems. Depending on the computational load of getContainers(), it might make sense to call that before calculating each k-coefficient.

The nature of RK4 integration requires a bit of a rethink into how the particle updates are parallelized. I've refactored both the Euler and RK4 updateVelocity functions to place the parallelization inside the scope of the functions themselves. Assuming I've done this correctly, this should now be able to correctly implement RK4, as well as providing additional flexibility if/when you decide to try more integrators.
Also fixes tabs/spaces inconsistency
@jacione jacione mentioned this pull request Jan 3, 2025
@jacione
Copy link
Author

jacione commented Jan 3, 2025

I also just realized that particlesBuffer is definitely used elsewhere in the code. I must have messed up the spelling when I ctrl-F'd it. And since it's used in getContainers, you'd probably want to just make a second buffer if you do end up recalculating the containers for each coefficient.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant