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

Limit initial time step to avoid control system deadlocks #5732

Merged

Conversation

wthrowe
Copy link
Member

@wthrowe wthrowe commented Feb 1, 2024

Proposed changes

Upgrade instructions

Code review checklist

  • The code is documented and the documentation renders correctly. Run
    make doc to generate the documentation locally into BUILD_DIR/docs/html.
    Then open index.html.
  • The code follows the stylistic and code quality guidelines listed in the
    code review guide.
  • The PR lists upgrade instructions and is labeled bugfix or
    new feature if appropriate.

Further comments

@knelli2 knelli2 self-requested a review February 1, 2024 22:45
Copy link
Contributor

@knelli2 knelli2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the BBH metavariables, the InitializeMeasurements action is at the end of the list, so it's fine that we change the step size there. But what prevents somebody from putting another initialization action after it that again changes the step size to something undesirable by the control system? Not saying this needs to hold up this PR, but something we should think about.

Comment on lines 152 to 153
TimeDelta new_step{};
if constexpr (Metavariables::local_time_stepping) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[optional] You could put the if constexpr inside the db::mutate. Avoids the intermediate variable.

Comment on lines 300 to 301
test_initialize_measurements<false>(true, true);
test_initialize_measurements<false>(true, false);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[optional]

  for (const auto& [ab_active, c_active] :
       cartesian_product(std::array{true, false}, std::array{true, false})) {
    test_initialize_measurements<false>(ab_active, c_active);
    test_initialize_measurements<true>(ab_active, c_active);
  }

Previous restriction was stricter than necessary.  Now uses the same
algorithm as Adams-Moulton.

Also replace the comment in AM with a clearer explanation.
Some operations (e.g., control system stuff) can't be done during
self-start, so it is good to keep the time range as small as possible.
@wthrowe wthrowe force-pushed the initial_control_system_time_step branch from 3e72d93 to 818a0ef Compare February 8, 2024 22:24
@wthrowe
Copy link
Member Author

wthrowe commented Feb 8, 2024

Initialization actions are only allowed to decrease the time step, otherwise they could destabilize the evolution. That won't interfere with this code.

The one exception I could imagine is a hypothetical initial run of some StepChoosers to pick a time step, but that's not really any worse than the existing restrictions on the step action ordering.

Copy link
Contributor

@knelli2 knelli2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

Avoids deadlocks during self-start if the requested time step is too
large.
@wthrowe wthrowe force-pushed the initial_control_system_time_step branch from 818a0ef to 13ecc12 Compare February 9, 2024 20:54
@nilsdeppe nilsdeppe merged commit 07eea61 into sxs-collaboration:develop Feb 12, 2024
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants