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

Test2 #102

Closed giovanni-rosotti closed 7 years ago

giovanni-rosotti commented 7 years ago

See #70 , moved to another branch to cleanup history. I think we are now finally ready to merge this branch! I was able to fix the hybrid test fiddling around with the openMP configuration, so that now all tests run under 10 minutes, which is pretty nice.

rbooth200 commented 7 years ago

Ok great. Can you explain a little why the last fix works so well?

giovanni-rosotti commented 7 years ago

Because in this way the openMP threads yield when they have finished instead than busy waiting. On an oversubscribed machine, this gives a chance to the other threads to finish their work. Otherwise they need to wait for the OS to schedule them.

rbooth200 commented 7 years ago

I guess this is only really beneficial in this case of over subscription rather than normal usage.

giovanni-rosotti commented 7 years ago

I guess this is only really beneficial in this case of over subscription rather than normal usage.

That's right. But there is no change to the code, only to the travis configuration; no need to worry for the general user

rbooth200 commented 7 years ago

Can we try adding back the dustbox test to the tests run? It's one that has a simple analytical solution. Or should we delay this for later work?

rbooth200 commented 7 years ago

Does this commit (d4e1e25) fix the problem with Npart=0 for the oct tree? Even if it is broken for another reason we shouldn't let it be broken in even more ways...

giovanni-rosotti commented 7 years ago

Can we try adding back the dustbox test to the tests run? It's one that has a simple analytical solution. Or should we delay this for later work?

It's just a one liner so I will do it. There's still no check for the correctness though.

Does this commit fix the problem with Npart=0 for the oct tree? Even if it is broken for another reason we shouldn't let it be broken in even more ways...

Which commit? The link doesn't work...

rbooth200 commented 7 years ago

I've updated the link...

giovanni-rosotti commented 7 years ago

Ok, I've done these two little things. I guess the question about the tree is for David really.

Incidentally, I am not satisfied with having two copies of DeleteDeadParticles as they are exactly the same, but this is another topic...

rbooth200 commented 7 years ago

I will create an issue for this

giovanni-rosotti commented 7 years ago

So my understanding from David is that the relevant changes have been done also the oct tree. Therefore I think this is now ready to be merged.

dhubber commented 7 years ago

ok, I just double-checked and I have made the same changes to both the BruteForce and the OctTree as I did to the KdTree. I also re-run the dustywave test in the virtual machine with the bruteforce and it worked fine. Since it's still broken, I couldn't obviously test the OctTree but the new additional Npart=0 code segments are visually correct. I will work on fixing the OctTree in a separate branch, so if that's everything with this one, I think we should be fine to finally merge!

rbooth200 commented 7 years ago

Ok, I agree that this is ready. merging