Apply output time cadence changes even if no input file is provided during restarts

[felker]

Makes command line changes equivalent to input file changes. Closes #451. Can you check @EdwinChan ? I just removed the if (iarg_flag == 1) check for the Forward/RollbackNextTime() calls in main.cpp, which I am pretty sure is safe in all cases but not 100% positive.

Also closes #310, by skipping the initial condition output if next_time > start_time. This was one of two suggested changes; see discussion in #311. The alternative is to stop overriding next_time when the IC is outputted, in other words "keep next_time after the initial output."

• [ ] Document this change

---

451 had many other comments and suggestions, which should be opened as separate issues if we want to pursue any of them:

• [ ] Expose outputN/file_number as a user-modifiable options, like outputN/next_time:

  ./outputs/outputs.cpp:137:      op.next_time = pin->GetOrAddReal(op.block_name,"next_time", pm->time);

• [ ] Give the user the ability to overwrite mesh->time (I suppose this only matters for restarts, since you can change time/start_time for fresh simulations)
• [ ] Policy change about next_time < pm->time behavior when next_time is changed via input file and/or command line during restart. I regard this as an invalid/ill-defined choice that the user has no reason to assume will result in a particular behavior , so I think the current implementation is fine. Currently we use ForwardNextTime() to set "the next output after the restart happens at next_time_new = next_time + n * dt so that next_time_new >= pm->time for the smallest such non-negative integer n"

The suggestion is that "the output should happen as soon as possible ..., because that would give us an easy way to make a debug dump right at the point of restart", i.e. next_time_new = pm->time like an initial condition output for the restarted simulation, which does seem useful.

Regardless of the choice, we should explicitly document it in the Wiki if it isnt already documented.

[EdwinChan]

Thanks, @felker, for the patch. The four tests in the original post of #451 have been redone with commit 08a002a0f057a390d320e3205d1615889f187d46; only the third test produces different results, and the new results are identical to the second test. That is to say, the specific problem I raised seems to be fixed. I'll let the maintainers contemplate the wider ramifications, if any.

I second the idea of documenting in the wiki the behavior when next_time < pm->time (as C++ programmers I'm sure we all hate undefined behavior…). Better yet, Athena++ can simply terminate on invalid input. As a bonus, we can teach the readers how to read the time in the restart file (see, e.g., https://github.com/PrincetonUniversity/athena/issues/451#issuecomment-1232344719) and set output#/next_time to that.

[felker]

Thanks for checking @EdwinChani. Can you draft an edit to the Wiki, and I will apply it?

[felker]

Any suggested edit to the Wiki, @EdwinChan ?

[EdwinChan]

I felt like I'm being gaslighted by GitHub… I was away during the long weekend and I can clearly remember replying to this thread after I came back to the office last Wednesday, but that post just disappeared. It's giving me serious trust issues…

Anyway, here's my suggested edit to the wiki:

• next_time (optional) : earliest time the simulation must reach before the next output; the behavior is undefined if next_time is earlier than time/start_time for an initial run (see <time> block below; default: time/start_time), or the time stored in the restart file for a restart run
• file_number (optional) : number by which the next output is labeled (default: 0; should satisfy 0 ≤ file_number ≤ 99999)

Some notes:

• "reach" means output is performed also when pm->time == next_time; "exceed" would exclude the equality case
• the behavior is still defined for the equality case, in which case the output should happen as soon as possible
• the way the main loop is currently written makes adhering to the specification laid out above difficult:
  • for initial runs, there's additional logic before the main loop that dumps the IC, but no time check is performed
  • for restart runs, the first output opportunity is after one iteration of the main loop, so the time is already different
• please check that my interpretation of the code gives the correct default value for output#/next_time
• the hint about the range of output#/file_number is to avoid formatting issues

[felker]

https://github.com/PrincetonUniversity/athena/wiki/The-Input-File

how does this look?

[EdwinChan]

The only quibble I have is that the phrase "to write the initial condition" implies that any other value of next_time would prevent writing of the initial condition, but in fact the initial condition is written unconditionally (pun?):

https://github.com/PrincetonUniversity/athena/blob/15bd136cd5237980965ce774d3ce4c5f1531980c/src/main.cpp#L399

I'd suggest removing that phrase.

[felker]

Not anymore: https://github.com/PrincetonUniversity/athena/commit/13ebdb9cf3ce23e64a7cae11f5540de7c52d3c33

https://github.com/PrincetonUniversity/athena/blob/15bd136cd5237980965ce774d3ce4c5f1531980c/src/outputs/outputs.cpp#L778-L790

[felker]

maybe good to add a comment in main.cp since the name of the function call is misleading

[EdwinChan]

For what it's worth, setting next_time to the time in the restart file means the next output will happen at next_time + dt, necessitating "exceed" in the description. Immediate output after restart is possible if next_time + dt is slightly later than the restart time.

[felker]

Are you suggesting I modify this?

• next_time (optional) : earliest time the simulation must reach (or exceed) before the next output is written; the behavior is undefined if next_time is earlier than either 1) time/start_time for an initial, non-restart run 2) or the time stored in the restart file for a restart run (default = time/start_time to write the initial condition for a non-restart run)

[EdwinChan]

Not really; the way you wrote it is loose enough to cover the situation I described. But someone years later might have Googled this page when trying to figure out why there is no output even though start_time is after the restart time, and I thought it might be worthwhile to mention it here.