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

Improve ics3 #178

Open dhubber opened 6 years ago

dhubber commented 6 years ago

Replacement pull request for previous improve_ics2 branch. Main changes :

I might add a few small commits in the next few days but this now works without generating the Travis errors it did before. I still did not fully understand what happened but now at least I have the debug code in place to catch it and understand it better if it happens again.

dhubber commented 6 years ago

ok, very very strange! The branch was passing the Travis tests and then I pushed a commit simply to add a few more entries to the gitignore and now it's failing? Even though the gitignore has nothing to do with the actual compiled code??

dhubber commented 6 years ago

See issue https://github.com/gandalfcode/gandalf/issues/181

dhubber commented 6 years ago

Partly this has been fixed due to a couple of small bugs being corrected but the radws was failing because of the fickleness of the test due to small changes (such as the hexagonal lattice code I changed). So I commented the radws assertions out to check everything else worked fine and it seems ok now. However, what should we do about the radws test (or indeed any other test that has similar problems in the future)? I'd like to either decide on a strategy in the future, or at the very least resolve this particular case, before the branch is merged.

rbooth200 commented 6 years ago

I was reluctant to add RADWS as a test this way at all since it was so fickle. But then again, we don't have any comparable test. Perhaps the most sensible option is just to relax the tolerances to anything within a factor of 2 or three of the expected result?

dhubber commented 6 years ago

Unfortunately, the test here has a tolerance of 0.1 and failing with ~0.5. It's just too sensitive, especially at the transition between the freefall collapse phase and the quasi-static collapse phase. I would have thought a better diagnostic to measure its accuracy is the locus of maximum (central?) density to maximum (central?) temperature, or perhaps even the central density against time, and compare either to a known tabulated solution. This would be a much less fickle diagnostic than the current approach which simply takes a single snapshot and would be much more in line with how we measure the accuracy of the freefall collapse test. I might have to ask Dimitri about this though.

rbooth200 commented 6 years ago

Central density vs time is not good at low resolution for sure -- that's essentially what we are testing. Central density against central temperature might be better, I'm not sure.