Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
2 tasks
felker opened this issue May 15, 2023 · 3 comments
Open
2 tasks

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

felker opened this issue May 15, 2023 · 3 comments
Labels
help wanted Need help from another Athena++ developer

Comments

@felker
Copy link
Contributor

felker commented May 15, 2023

(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

@felker felker added the help wanted Need help from another Athena++ developer label May 15, 2023
@c-white
Copy link
Contributor

c-white commented May 15, 2023

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
Copy link
Collaborator

pgrete commented May 16, 2023

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
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Need help from another Athena++ developer
Projects
None yet
Development

No branches or pull requests

4 participants