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 eos - Radws #171

Closed rbooth200 closed 6 years ago

rbooth200 commented 6 years ago

This pull request includes changes that refactor the meshless to work with an arbitrary equation of state.

I've tested this this will the EOS that includes H2 dissociation and H + He ionization from Stamatellos+ (2007), which makes up the 'EOS' part of the radws scheme. I've also included an extra shock test to check that this is working. Since there are no analytical solutions I just compare the SPH and meshless results. Given that we already test the EOS itself with the cloud collapse test, I think this should be satisfactory.

rbooth200 commented 6 years ago

This PR is ready to go now.

dhubber commented 6 years ago

ok, I've got 2 main queries before accepting this (sorry for the delay; there's a lot to catch up on and I'm on holiday this week :-) ) 1) I was a bit concerned about introducing yet another particle data structure (EosParticleProxy), especially if it meant changing the interface of so many functions (such as the EOS functions). However, if I understand it correctly, you can still pass a Particle data structure to these functions which then constructs an EosParticleProxy instance on-the-fly (via EosParticleProxy(Particle& p) ) and then works as normal? If so, then I guess this is fine since it seems most of the changes are contained in only the EOS and Meshless regions of the code. 2) The RadWS scheme now has an automatic Travis/nose2 test I believe from the previous branch that was merged? I saw a 'Radws' test in the Travis output which I assume is this collapse test and therefore the meshless version of the code also passes this test? You say here it agrees with the SPH but I assume the SPH itself gives acceptable results already compared to the Masunaga-Inutsuka collapse test? (I think this was all discussed in the previous Radws branch a lot so just verifying).

rbooth200 commented 6 years ago

Hi David,

1) You are correct here, the object is constructed automatically. The compiler can probably do away with this entirely in some special cases too. Otherwise the reason for this is to make it obvious to people (i.e. me) who need to get the thermal properties at locations away from particles what properties they need.

2) This branch does not include the cooling that is needed for Masunaga-Inutsuka collapse test so I can't use it here, hence the shock test. I've done that bit in the next pull request, where it does agree.

rbooth200 commented 6 years ago

I did also use a test which is similar to one in the PLUTO user guide, and the results seem to agree, so I am happy here.

dhubber commented 6 years ago

ok, I was about to accept but I see there's now some conflicts (I guess with the gravity branch that was merged the other day). I'll accept once they're resolved and it passes Travis.

rbooth200 commented 6 years ago

Ok, I've resolved the conflicts. Hopefully this will work now

rbooth200 commented 6 years ago

Ok, the merge went fine, this is ready to go.