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

Made rdmdt conservative #150

Closed rbooth200 closed 6 years ago

rbooth200 commented 7 years ago

I made the gravitational update now use the conservative change in the centre of mass with the MFV scheme. However, it still does not work well...

dhubber commented 7 years ago

ok, so guess there's something more fundamental going wrong. The mass-flux must in general be correct or we'd be getting crap with the MFV hydro tests. The tree updates from the particle properties; are we updating those back correctly or doing it inconsistently so the tree is wrong also? I might have a dig around in the code at some point (after all my paper figure tasks are done).

rbooth200 commented 7 years ago

I think we are resetting the particle masses after 'predicting' their positions (AdvanceParticles), so that should be ok. We are also using the full dQ in the prediction when we get to the end of the time-step (which is when we do the gravity) so I think it should all be consistent...

giovanni-rosotti commented 7 years ago

What about this? Is it in improve_openmp2? (can't find it there, so I assume not). David? It should be up to you to accept this since you are more familiar with the meshless

rbooth200 commented 7 years ago

It's not in improve_openmp2. I'll let David decide to do. In my opinion these changes should be objectively better, but I don't see it helping in the tests.

dhubber commented 7 years ago

No, I don't think I merged this with improve_openmp2 (I just checked and couldn't find the commit anyway).

dhubber commented 7 years ago

Yes, I think we should get to the bottom of the MFV gravity issue before merging. I'm still not sure if something is implemented wrong or if it has some fundamental issues. So I'd suggest leaving this for now. Maybe just add a small fix to throw and exception is self-gravity is switched on with MFM until we can decide how to proceed?

rbooth200 commented 7 years ago

Ok, I've banned the MFV and self-gravity, but probably we shouldn't use it either if stars are present, even without self-gravity.

giovanni-rosotti commented 6 years ago

I'm confused by this branch. What's the point of this modification, if it only does something with self-gravity, but we don't allow to use it with self-gravity? (do I understand correctly?)

dhubber commented 6 years ago

MFV with self-gravity doesn't work but MFM does work (we think anyway).

Ok, I've banned the MFV and self-gravity, but probably we shouldn't use it either if stars are present, even without self-gravity.

Hmmm, not sure I understand this point though. Isn't the issue the self-gravity terms between neighbouring particles leading to mass flux which can lead to mass becoming negative? And so this will not happen with only star-gravity (unless I'm forgetting some other term)?

Once we resolve that question, I think we can look at merging perhaps?

rbooth200 commented 6 years ago

The rdmdt terms affect simulations with any kind of gravity - either stars or self-gravity. I don't think we should be using the MFV with either really.

giovanni-rosotti commented 6 years ago

but then, isn't this modification useless?

On Wed, Oct 4, 2017 at 3:34 PM, Richard Booth notifications@github.com wrote:

The rdmdt terms affect simulations with any kind of gravity - either stars or self-gravity. I don't think we should be using the MFV with either really.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/gandalfcode/gandalf/pull/150#issuecomment-334175716, or mute the thread https://github.com/notifications/unsubscribe-auth/ABdSgjEa3ViTVmDFvsftr2BGGpRJ1SR4ks5so5ddgaJpZM4NyYvJ .

dhubber commented 6 years ago

The rdmdt terms affect simulations with any kind of gravity - either stars or self-gravity. I don't think we should be using the MFV with either really.

Okay but you're talking about only the MFV right? The rdmdt terms should all be zero with the MFM because there is no mass-transfer allowed surely? If so, then this should be fine to impose in the code.

dhubber commented 6 years ago

but then, isn't this modification useless?

Well, depends if we think the rdmdt terms will ever work with self-gravity and MFV. They work in the GIZMO paper but we've already found plenty of instances where it's not clear how or why things work for him and not us. But this branch also has the code to disable MFV with self-gravity so at least that should part of it should be merged.

giovanni-rosotti commented 6 years ago

Ok, agree to accept the code to disable MFV then. And about the rdmdt modification, then if we ever switch on gravity with MFV again, it's because we will change those terms again anyway. So two things: a) the userguide should state that MFV should not be used with gravity b) the code should not run even if including stars with MFV. I didn't understand if David agrees with this, but if he does, then please make the code refuse to run if the user tries.

David or Richard, if you do these modifications I'd be happy to accept the branch

dhubber commented 6 years ago

I didn't understand if David agrees with this, but if he does, then please make the code refuse to run if the user tries.

Sure, I agree with both of those. I'll do those now then so we can be done with this pull request.