search

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

Mfv dust2 #144

Closed rbooth200 closed 7 years ago

rbooth200 commented 7 years ago

This branch updates the dust implementation, introducing several improvements:

To do this I have had to make a number of modifcations along the way, hence such a big pull request. Specifically:

I think these changes are now ready to go.

dhubber commented 7 years ago

Both KDK and DKD leapfrog are now properly implemented with SPH

Oh, was there a bug in the old implementation since it seemed to be working fine with me (or at least the KDK since I was using that one mainly)? Or do you mean it's all consistent with your new changes to the integration schemes?

To do this I have had to make a number of modifcations along the way, hence such a big pull request

Yes, it might be a bit tricky to review everything (although I can see many are effectively replace alls like the set/unset flag commands), especially since I don't really know much about the dust code. I think seeing as it passes all the Travis tests, I'll just run a few tests manually just to satisfy myself and then I'll be fine with merging (although I'll wait for Giovanni's okay also).

rbooth200 commented 7 years ago

Before only the DKD dust was implemented correctly. If you ran KDK you'd probably still get something sensible because the code would have still reproduced the correct limits of very small and very long stopping times. However, in the middle it was formally incorrect.

Other than that the changes were mostly straight swaps. The block time step code has actually been changed slightly: it now only computes the timestep for the next step, leaving the user to select it when ready...

dhubber commented 7 years ago

Before only the DKD dust was implemented correctly. If you ran KDK you'd probably still get something sensible because the code would have still reproduced the correct limits of very small and very long stopping times. However, in the middle it was formally incorrect.

Okay, so you're talking just about the dust algorithm. No problem then!

I had a short look at the code and run some (non-dust) tests myself manually just for peace of mind. If everyone else is happy, we can merge asap.

giovanni-rosotti commented 7 years ago

Hi both, the dust had some horrible bug with the sink accretion invalidating the tree when producing a snapshot at the end of the main loop (because of the call to DeleteDeadParticles). I've corrected this for SPH, and it seems to me that the meshless is already fine (please check!), because it always rebuilds the tree after sink accretion.

David, you should know that this makes the tree invalid when calling the supernova driver routine at the start of the following timestep. I don't know if that routine uses the tree, or if it ever will, but this is something you should be aware of.

rbooth200 commented 7 years ago

The meshless looks ok RE the dust as far as I can tell.

dhubber commented 7 years ago

David, you should know that this makes the tree invalid when calling the supernova driver routine at the start of the following timestep. I don't know if that routine uses the tree, or if it ever will, but this is something you should be aware of.

Yes, unfortunately it does call the tree when injecting supernova (since it needs to know surrounding neighbouring particles in case they need to be heated up also). And then once the new particles are injected, it makes sense to immediately rebuild the tree with the new particles included. So this definitely is a problem then since it was called from the ideal place before and now that doesn't work I guess. I might have to figure out if it can be moved easily, perhaps before the sink particles are called?

rbooth200 commented 7 years ago

I'm not sure its such a problem for the SPH. The reason it arises with the dust is that dust is much more likely to enter patholigical particle arrangements, finding far too many or no neighbours. So I added some extra logic to help when no neighbours are found in an iteration, i.e. set h = hmax and iterate, but that violated the clause...

giovanni-rosotti commented 7 years ago

I noticed that restarts aren't working with the dust (at least, after restart the timestep goes to 1e-10, so something crazy has happened!). It might not be the whole story, but I think the code we have now is clearly wrong. When we write a snapshot, we just record how many particles are gas and how many are dust. Then when we read the snapshot back in, we just say that the first Ngas particles are of gas type, and remaining of dust type. Now, this is correct if the initial conditions have been set up in this way, and if the order of the particles is maintained. But since there is accretion, this is not the case... Can't we simply write ptype in the snapshot file? Is there any reason why we are not doing it already?

rbooth200 commented 7 years ago

I think the reason is just that I never thought about what happens with sinks. I would have no problem writing the ptype, but I think we already have a solution when using MPI, which, if I remember correctly, involves sorting the particles by type when writing them to disc. Would this be too difficult?

I suspect that if we just write the ptype we are going to break everybody's seren readers...

rbooth200 commented 7 years ago

Ok, I so the read/write issue has been fixed in another branch. Is there anything else left that needs doing before this one can be merged?

dhubber commented 7 years ago

I suspect that if we just write the ptype we are going to break everybody's seren readers...

Actually it's designed to skip through any unrecognised arrays (as long as it's formatted correctly) in both gandalf and seren, so this should not be a problem to add an extra int array. But if the fix you've recently made has already accounted for this, then no need to do it anymore I guess!

dhubber commented 7 years ago

I just fixed some conflicts with master (and fixed a small other bug with VERIFY_ALL). I guess this is ready to go then, unless there's something we've discussed about adding that I've forgotten about??

rbooth200 commented 7 years ago

I think this is ready to go, too.

It might be easier if we merge the visc pull request first, to avoid any potential git issues...

dhubber commented 7 years ago

ok, I see you've merged this one into visc so guess we can close this pull request and delete mfv_dust2 now? I'll just wait until Travis gives the green light!

rbooth200 commented 7 years ago

No, merge the visc pull request and then the mfv dust one.

On Mon, 29 May 2017, 10:33 dhubber, notifications@github.com wrote:

ok, I see you've merged this one into visc so guess we can close this pull request and delete mfv_dust2 now? I'll just wait until Travis gives the green light!

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/gandalfcode/gandalf/pull/144#issuecomment-304605219, or mute the thread https://github.com/notifications/unsubscribe-auth/AOqItOrV8-_68kvUxdb-PAVKyMbYs05aks5r-oLOgaJpZM4NZNvo .