gandalfcode / gandalf

GANDALF (Graphical Astrophysics code for N-body Dynamics And Lagrangian Fluids)
GNU General Public License v2.0
44 stars 12 forks source link

Newparticles #73

Closed dhubber closed 7 years ago

dhubber commented 7 years ago

Introduces ability to add new particles on-the-fly during a simulation, principally via a new 'CreateNewParticle' function in the Hydrodynamics class. For now, this then triggers a full tree rebuild and ghost particle creation step (since the new particles are added at the end of the real particles in the array where the ghosts would otherwise live). The particles have to be added to a specific part of the loop (just before tree-rebuild/MPI load balancing); otherwise can cause problems. Other smaller additions

Outstanding issues :

giovanni-rosotti commented 7 years ago

I've left a couple of comments for the CreateNewParticle function, but I think it's fine. I have no idea about the supernova, so I can't really judge that part, neither the Ewald stuff... But definitely we should just crash if the user is trying to do supernovae with MPI. I'm also worried by the crash, I can imagine you will produce nonsense but it's pretty hard to make SPH crash. What is happening exactly?

dhubber commented 7 years ago

Yes, I just replied to those comments. Regarding the crash, I'm a little worried about it also but I'm not 100% sure what was the cause and if it's something we've fixed in our recent spate of bug-fixes. Maybe once we've merged the latest master branch with it I can try again and see if I still get the crash or not.

Speaking of merging, I'm slightly confused since git is complaining that there conflicts with this branch that were not there before. Is that because now the master branch has changes which are in conflict with this one (so the conflict was generated elsewhere) or did one of you guys try to merge master into this branch and generated conflicts here (although it doesn't say anything about that here)?? Anyway, I can resolve those conflicts later on if required since I should probably check myself if the new particles branch is still working fine with all these new changes.

rbooth200 commented 7 years ago

I think this pull request it waiting to be brought up to date with master after merge of reallocation.

dhubber commented 7 years ago

ok, getting cheesed off with git now. It seems to be getting confused with extreme ease over any little thing when rebasing/merging and causing a total mess in the commit history (done yet again here). So I'll have to close this branch and cheery-pick onto a clean branch yet again!! >.<

rbooth200 commented 7 years ago

Hi David, from looking at the network I think you added a bunch of the commits twice.

I think all that should have been necessary was to take to this branch, rebase on to master and then do a forced update (push -f). The error appeared during the merge following the rebase I think.

I'm going to try to clear this up by rolling back to commit 499c9aa. If there are any commits missing that you tried to bring in with the merge, then I suggest you cherry-pick them.

rbooth200 commented 7 years ago

Ok, the results of that in are in the newparticles_rebase branch. If it looks ok to you then it might be worth using that branch instead and cherry picking any changes you need.

dhubber commented 7 years ago

Hi Richard. No, I didn't add commits twice. This happened during the rebase onto master where it seems to remerge old commits that have already been added. I don't know why it does this all the time but it's getting really annoying now! Maybe I'm doing something wrong but I don't think so. Anyway, thanks for your help with this.

rbooth200 commented 7 years ago

It can also happen if your local branch is not up to date with the branch on github. If newparticles_rebase looks ok then you can fix newparticles by doing the following:

git checkout newparticles git pull git reset 499c9aa git push -f

Obviously this can fail if you have local unmerged files...

dhubber commented 7 years ago

ok thanks, will try that now.

dhubber commented 7 years ago

ok, the branch seemed fine at that commit so I've pushed it now and it's a lot cleaner thankfully.

dhubber commented 7 years ago

So, I think I've fixed the remaining issues with this branch (except for the MPI one). The 'NewParticles' routine now calls the new (re)allocate method to ensure the arrays are expanded if needed.

Also, I've found the cause of the crashes and fixed it. It was due to the Leapfrog KDK correction terms causing the internal energy to negative. Basically the correction term/full-step term is of the form u = u0 + 0.5_(dudt0 + dudt)_dt. The timestep is only calculated at the beginning of the step from dudt0, so if at the end of the step, there is a sudden huge negative dudt, then it can cause u to become negative. To 'correct' this, I simply switch to first order (i.e. u = u0 + dudt0*dt) if the second order would otherwise cause u to go negative. This kind of thing is only likely to happen around strong explosions or energy sources like the Sedov test.

This branch should now be (almost) ready for merging, unless we want to get this working in MPI at this point also!

giovanni-rosotti commented 7 years ago

That's fine - I'm not worried by the failing back to first order if it's only in rare occasions.

I think we do want the code at least to crash if using MPI though - letting the code run and consciously produce wrong results is very dangerous. You'll never know what the user is going to do with the code...

giovanni-rosotti commented 7 years ago

See #104 . Closing this pull request.