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

Newparticles2 #104

Closed giovanni-rosotti closed 7 years ago

giovanni-rosotti commented 7 years ago

See #73 . Doing it in a new branch to cleanup the history. Besides improving comments in a couple of places, I've added the supernova driver call also in the meshless main loop for consistency. I also got rid of the newParticles flag. The reason is that we already have a rebuild_tree flag, and it was not clear that new flag was introducing anything new. This new way is safer because we can use the existing checks for rebuild_tree.

I believe this is now ready to be merged.

giovanni-rosotti commented 7 years ago

Sorry, I was forgetting about the last commit. The code now crashes with MPI. I discussed the thing with David and we think that it is premature to implement supernovae with MPI at this stage - things might still change. It's best to wait and do it later in the icsilcc branch.

dhubber commented 7 years ago

ok, I made my changes and pushed them but as you perhaps can see there are conflicts with master. so I rebased with master and now getting the usual crap where I'm apparently behind the remote version. So, am I fine in this instance to do a forced-push as Giovanni suggested (git push -f origin newparticles) so as to not mess up the history?

rbooth200 commented 7 years ago

Its generally necessary to do a forced update after a rebase. Check with gitk first that the commit history looks ok, if so then go ahead...

giovanni-rosotti commented 7 years ago

Ok - I can confirm David that the rebase is correct, this branch now stems from master. It's up to Richard though to say whether he is satisfied with the changes.

rbooth200 commented 7 years ago

Is the periodic correction for the nbody correct? We don't seem to be updating adot.

With regards to CreateNewParticles I'm not convinced the current solution enforces correctness either, since there are many caes in which the current particle initialization will not work. For example, if we included chemistry or anything else that requires values to be initialized to a given range. Given the choice of that function returning something that is always wrong versus sometimes wrong I think the best solution is always wrong - then the caller HAS to do think about what they are doing. Allowing them to not think and create a possibally invalid particle is a really bad idea and it will bite us in the future.

Ideally we should go further than this: The calling functions that initialize the particle should only exist for particles that they are valid for, i.e. the supernova driver should fail if the particle is not SPH unless e.g. MFV particles are properly supported.

giovanni-rosotti commented 7 years ago

Is this still useful now? Or have all these commits gone into final_paper_tests?

dhubber commented 7 years ago

I'm pretty sure all the commits have gone into final_paper_tests but I want to check the commits in turn to be sure before deleting. I'll go through this on the weekend and close it if I'm happy it's unnecessary.

dhubber commented 7 years ago

Finally checked through these commits and seems like there's nothing here not already in master (since icsilcc and then final_paper_tests originally came from this branch). So, I'm going to close this commit.