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

Convert initialization action for time to mutator #4513

Merged
merged 1 commit into from
Dec 16, 2022

Conversation

kidder
Copy link
Member

@kidder kidder commented Dec 13, 2022

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

@kidder kidder requested a review from wthrowe December 13, 2022 20:18
wthrowe
wthrowe previously approved these changes Dec 14, 2022
/// and `Tags::Next<Tags::TimeStepId>` is the initial time.
template <typename Metavariables, bool UsingLts>
struct TimeStepping {
using time_stepper_t =
Copy link
Member

Choose a reason for hiding this comment

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

[optional] This is a "traditional" type alias, not a metavariable, so should be camel case.

Copy link
Member Author

Choose a reason for hiding this comment

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

I will do this change now

nilsdeppe
nilsdeppe previously approved these changes Dec 15, 2022
/// Since the evolution has not started yet, initialize the state
/// _before_ the initial time. So `Tags::TimeStepId` is undefined at this point,
/// and `Tags::Next<Tags::TimeStepId>` is the initial time.
template <typename Metavariables, bool UsingLts>
Copy link
Member

Choose a reason for hiding this comment

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

We could also pass in the step_chooser_simple_tags directly instead of the Metavariables, but I'm totally fine doing that later in order to get this merge :)

Copy link
Member Author

Choose a reason for hiding this comment

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

I will do something later when I deal with step_chooser_compute_tags (which are now handled by calling AddComputeItems, but should be able to just add them to one of the initialization mutators)

Copy link
Member

Choose a reason for hiding this comment

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

See also #4484.

@kidder kidder dismissed stale reviews from nilsdeppe and wthrowe via a71e3df December 15, 2022 21:02
@kidder kidder force-pushed the initialize_time_step branch from 4f28b96 to a71e3df Compare December 15, 2022 21:02
nilsdeppe
nilsdeppe previously approved these changes Dec 15, 2022
@nilsdeppe nilsdeppe enabled auto-merge December 16, 2022 18:29
@nilsdeppe nilsdeppe merged commit 0130467 into sxs-collaboration:develop Dec 16, 2022
@kidder kidder deleted the initialize_time_step branch January 9, 2023 22:30
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