Add a global time_delta_coeff control for binary timestep controls

[orlox]

For general convergence testing it would be good to implement this functionality of star also in binary. It should also catch up any environment variables defined to increase resolution.

[matthiasfabry]

I've implemented the required flag on the linked branch, but I'm unsure on how to systematically test this.

[warrickball]

cannon_conroy_intel automatically tests all branches, so this is already being picked up TestHub. You've used [ci skip] in the commit message, though, which means cannon_conroy_intel (and any other system that's set up to recognise the commit message flags) only builds MESA and doesn't run the tests. These flags are listed here.

Another good practice is to run locally any relatively quick tests that you think may have been affected. I do this in the astero module, to at least make sure that the tests will start. (I loop over the tests and run them with timeout 30s ./rn or similar.)

I've just submitted a run to bluebear_ifort now, and results for that branch should start coming back in about 30 minutes. bluebear_ifort isn't set up to run automatically or detect the commit message flags.

[matthiasfabry]

I wasn't referring to how to actually running the test suite, but rather to how we'd design new tests to see if this works as intended. No test in the binary suite is directly impacted (ie there's no new physics or something and it's a new control), but of course all tests are indirectly impacted since they have to pass through binary_pick_new_timestep.

So I guess you're right in that we should test whether all current testcases still give consistent results, but then again we'd need to design something that verifies our new control works as intended.

[orlox]

I presume the behavior we'd want to see from the testhub now is that there was no change at all from the old behavior. If we want to have a semi-automated validation of this, we would need a new test case that does a run twice. That could be done in conjunction with time_delta_coeff and mesh_delta_coeff. By doing a run with defaults, and a run with all these set to 0.5, one could check that to a certain tolerance the number of steps and the resolution by the end of the simulation doubled.

[warrickball]

Aha, I apologise for misunderstanding.

Could this be something for a unit test, rather than integration test? e.g. call binary_pick_next_timestep with 1.0 then 0.5 for time_delta_coeff and check the step is divided by 2? If so, we could do this in star too. It might be a bit of a nuisance to set up the star_info and binary_info pointers correctly though.

[matthiasfabry]

I concur this warranting a unit test. Does this go into test/test_binary.f90 or elsewhere? If yes I can start designing a simple routine.

[warrickball]

I don't think we have a clear practice for how to construct unit tests and we should probably generally use them more.¹ Some exist, especially in the "lower-level" packages, so you can probably cobble together something by looking at some of the microphysics packages (e.g. eos and kap) that have more substantial tests than e.g. star. I just glanced at binary and astero and they don't seem to test anything at all! :grimacing:

If you devise a reasonable way of doing things, it could be a model we follow to flesh out some unit tests in those modules.

To be clear, this would be great but I don't think should hold up merging #501.

¹ I'm 100% guilty of this myself. The surface_effects test case in astero is mostly a unit test on the surface correction routines.

[matthiasfabry]

I've made a very simple check on b% fr that when you divide b% time_delta_coeff by 2 you get half the calculated timestep.