PrincetonUniversity / athena

Athena++ radiation GRMHD code and adaptive mesh refinement (AMR) framework
https://www.athena-astro.app
BSD 3-Clause "New" or "Revised" License
233 stars 124 forks source link

Clean up time integrator task list; copy Parthenon style #502

Open felker opened 1 year ago

felker commented 1 year ago

(As discussed at the 2nd Athena++ developer workshop)

Not exactly a "good first issue", but certainly doable for a short student project, e.g.

https://github.com/PrincetonUniversity/athena/blob/master/src/task_list/time_integrator.cpp is an unreadable mess that is hard to extend or modify. In my opinion, 4th order hydro, orbital advection, and especially shearing box made the current setup untenable.

492 and #462 should not make the situation worse, but add separate task lists for Newtonian radiation and chemistry networks. @c-white also mentioned upcoming minor improvements to the time integrator task list?

But in the spirit of give-and-take in the Athena++ <-----> Parthenon pipeline, we should adopt (steal) several task list improvements from Parthenon, see: https://github.com/parthenon-hpc-lab/parthenon/blob/develop/src/tasks/task_list.hpp

c-white commented 1 year ago

That somewhat helpful strategy I employed in the GR radiation branch was to reduce the depth of nested conditionals. Even more of this can be done.

It also helps to make auxiliary variables for flags to be accumulated in different conditionals. For example, PROLONG needs dependencies on SEND_SCLR and RECV_SCLRSH whenever NSCALARS > 0, so this should be done once, accumulating into prolong_req, rather than under every combination of other physics that affects the PROLONG dependencies. Much of what seems to be combinatorial complexity in nested conditionals actually separates like this into sequential conditionals with linear complexity in the number of physics modules.

pgrete commented 1 year ago

FWIW printing out a graph should be straightforward if an API similar to the one in Parthenon is used (as all information could be printed in a way understood by some graph generating lib).

tomidakn commented 1 year ago

With or without TaskList, I think any code with many physical processes more or less suffers from the same problem. I agree with @c-white that we should be able to refactor the TaskList construction.

I also agree that it is straightforward to visualize TaskList dependencies using some graph generating package. As this is useful anyway, I think this is a good point to start with.

On the other hand, TaskList is not frequently changed by users or even by developers. It matters only when adding a new physics module. So while it sounds like a big issue, the actual impact on users is probably quite limited.

The iterative task and more flexible task API are nice features to have, but I guess there is no strong demand right now.