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

Corrected writing of file format with dust when sinks are present for… #147

Closed rbooth200 closed 7 years ago

rbooth200 commented 7 years ago

… su and sf formats

The particles are now always sorted by type in the su/sf formats. I haven't changed the column format since it isn't supporring different particle types anyway.

dhubber commented 7 years ago

So, does this now skip over any dead/accreted particles when writing the files? What we'd like to do of course is remove that call to DeleteDeadParticles after sink accretion only for writing files (the one Giovanni cited) which would break other things. I was thinking of a plan to get rid of this myself, but if this does it already, then great!! You mention dust with sinks explicitly, but didn't know if this also included sink accretion in general.

rbooth200 commented 7 years ago

Actually, it doesn't skip the dead particles, but I can certainly add it in... I'll do that later

dhubber commented 7 years ago

Okay, I think I can see how to skip the dead particles if you've not done it already (and if you have please push). It'll require printing out the number of live hydro particles (not just Nhydro) and then skipping in these new 'Write' functions if the dead flag is active. I can also add in writing and reading the ptype while I'm at it!

dhubber commented 7 years ago

Actually, skip writing and reading ptype since the way you've added seems to do the trick and will simply overwrite any values anyway! So guess there's no need for that anymore!

rbooth200 commented 7 years ago

Sounds good. If you have time please do it, i haven't got there yet...

On Tue, 23 May 2017, 09:37 dhubber, notifications@github.com wrote:

Actually, skip writing and reading ptype since the way you've added seems to do the trick and will simply overwrite any values anyway! So guess there's no need for that anymore!

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

dhubber commented 7 years ago

Okay, I've implemented the main parts of ignoring dead particles. It seems to run okay with sinks (during the Boss-Bodenheimer test with Grad-h SPH) apart from some problem that I've already fixed on another branch (and will make a pull request soon). A few extra things to do :

dhubber commented 7 years ago

ok, MPI clearly not yet working with Travis. Will look at that now.

rbooth200 commented 7 years ago

This is looking good as long as it passes the travis tests. However, I do think its necessary to make the column format explicity skip the dead particles like we are doing with the other format...

dhubber commented 7 years ago

This is looking good as long as it passes the travis tests. However, I do think its necessary to make the column format explicity skip the dead particles like we are doing with the other format...

I seem to be getting some assertion failures in the HydroTree::FindMpiTransferParticles function (in the big debugging block near the end of the function). Was thinking they were possible out-of-date/incorrect assertions but actually without them the code crashes with some memory/malloc error later. So something else to fix! :-/ Although so far, no problems with Travis.

Yes, the column format should also do this but just getting the difficult/important one out of the way!

rbooth200 commented 7 years ago

Yes, the column format should also do this but just getting the difficult/important one out of the way!

Ok

I seem to be getting some assertion failures in the HydroTree::FindMpiTransferParticles function (in the big debugging block near the end of the function). Was thinking they were possible out-of-date/incorrect assertions but actually without them the code crashes with some memory/malloc error later. So something else to fix! :-/ Although so far, no problems with Travis.

As far as I can tell you haven't removed the delete dead particles from the main loop on yet, so I would expect it to work just fine here. Maybe you have some other local changes that are making the difference?

rbooth200 commented 7 years ago

Whats the status of this now?

dhubber commented 7 years ago

Nothing more today sorry since it's a holiday in Germany and actually the weather is nice for a change :-). I was planning on looking at it later when I'm back home.

On Thu 25. May 2017 at 12:42, Richard Booth notifications@github.com wrote:

Whats the status of this now?

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/gandalfcode/gandalf/pull/147#issuecomment-303980425, or mute the thread https://github.com/notifications/unsubscribe-auth/AEsg1dfNbqW7VFzVpp_D4w7PMJaH_nXgks5r9VsdgaJpZM4Nf4WW .

dhubber commented 7 years ago

ok, I've had a look this evening and still problems. I just commented out the delete dead particles bit to check it with Travis but it's still failing asserts with the Boss-Bodenheimer test on my laptop. In particular, it's failing on line 1519 in HydroTree.cpp (some verification block that's looking for edge cases). Whoever wrote this bit perhaps can look at it and see if there's anything wrong, or it's catching a real problem? If I switch off debug mode, it still eventually crashes in a different place. But first things first. I'll get back to my other tasks in the meantime.

rbooth200 commented 7 years ago

Was the bug introduced introduced in this branch, or is it in the master? If its in the master can you also check whether its present in the visc branch or mfv_dust2?

dhubber commented 7 years ago

Sure, good shout, I'll check the other branches now since I didn't check them recently with MPI on my laptop.

dhubber commented 7 years ago

ok, the master branch has the same assertion failure on the same line! Will see if I get the same error in Linux now just in case it's a Mac thing!

rbooth200 commented 7 years ago

Ok. So I think this branch should be ok to merge. Can you remove the commented out code entirely? Perhaps Giovanni, should look over it first.

dhubber commented 7 years ago

ok, just done that although this is only for SphSimulation (will need to do the Meshless also assuming it has a similar delete particle line). I assume you mean Giovanni should look before we merge?

rbooth200 commented 7 years ago

I assume you mean Giovanni should look before we merge?

Yep.

I think we'll need to remove these lines again in the mfv_dust2 branch because I moved them around. But I can worry about that anyway.

When you have time can you look at the visc branch?

dhubber commented 7 years ago

When you have time can you look at the visc branch?

Sure, you mean as in look at it with a view to merging rather than testing for this bug/issue in that branch also??

rbooth200 commented 7 years ago

The first...

giovanni-rosotti commented 7 years ago

Ok trying to catch up with this long conversation... do I understand correctly that you are asking me to check at line 1519 of HydroTree.cpp? The check seems correct to me. If it fails, it probably means we have a problem either with the tree, or with the domain boxes. That line is checking that if we have a particle outside of the local domain, we are exporting it to some other domain. I am happy to debug this (although maybe it should happen in a separate branch). What are you running exactly?

dhubber commented 7 years ago

I am happy to debug this (although maybe it should happen in a separate branch). What are you running exactly?

Well, that's up to you but this branch is still pretty small so can be done here if you want. I ran the Boss-Bodenheimer test (the one in tests/astro_tests should do fine) in MPI (no OpenMP) on just 2 cores on my laptop (and with COMPILER_MODE = DEBUG and DEBUG_LEVEL = 1).

rbooth200 commented 7 years ago

Since this is a bug in the master I think that this branch should be judged on its own merits, and David's issue tackled in a seperate branch

giovanni-rosotti commented 7 years ago

Do I understand correctly that this is ready to go? If yes, I am happy for it to be merged - the MPI issue deserves a separate branch and issue as said.

rbooth200 commented 7 years ago

I think it is

On Sat, 27 May 2017, 17:18 giovanni-rosotti, notifications@github.com wrote:

Do I understand correctly that this is ready to go? If yes, I am happy for it to be merged - the MPI issue deserves a separate branch and issue as said.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/gandalfcode/gandalf/pull/147#issuecomment-304461632, or mute the thread https://github.com/notifications/unsubscribe-auth/AOqItIM3QdTj2PcQCpsHGceOnmgAEc4xks5r-EzogaJpZM4Nf4WW .

giovanni-rosotti commented 7 years ago

Ok, I was about to merge but noticed that the meshless main loop still contains a call to DeleteDeadParticles. Is there a reason for that? If not, I'll get rid of that. Btw, now the SPH main loop (and I guess the meshless as well very soon) does not call DeleteDeadParticles any more; it happens only at tree rebuild. I guess this is the best; in this way we don't break the tree by accident, and the main loop does not see such a low level operation.

rbooth200 commented 7 years ago

That's best. I think the motivation for not fixing the meshless was that we will need to fix it again in the dust branch. But either way it's fine...

On Sat, 27 May 2017, 18:27 giovanni-rosotti, notifications@github.com wrote:

Ok, I was about to merge but noticed that the meshless main loop still contains a call to DeleteDeadParticles. Is there a reason for that? If not, I'll get rid of that. Btw, now the SPH main loop (and I guess the meshless as well very soon) does not call DeleteDeadParticles any more; it happens only at tree rebuild. I guess this is the best; in this way we don't break the tree by accident, and the main loop does not see such a low level operation.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/gandalfcode/gandalf/pull/147#issuecomment-304465646, or mute the thread https://github.com/notifications/unsubscribe-auth/AOqItLoWxAqqo8f1gCbmfw6kOeC1cKKTks5r-F0ZgaJpZM4Nf4WW .

giovanni-rosotti commented 7 years ago

In this case I'll accept this as it is.