Skip to content

Conversation

@wthrowe
Copy link
Member

@wthrowe wthrowe commented Feb 14, 2025

Simplifies control flow by getting rid of jumping around the action list. Will simplify some changes to the step-size logic in an upcoming PR.

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

nilsdeppe
nilsdeppe previously approved these changes Feb 17, 2025
@wthrowe
Copy link
Member Author

wthrowe commented Feb 18, 2025

CI appears to have run out of memory. I don't think there's any reason this PR would significantly increase compiler memory use, but we may have been on the edge before and a small increase tipped it over or something. Although it's odd that it happened on all three compiler versions.

I don't see any obvious easy savings in ComputeTimeDerivativeImpl.tpp. (Unsurprising, since I'm not the first to look for them.) I may try splitting the instantiations across more files.

To replace the remaining uses of the ChangeStepSize action.
And move the remaining change_step_size function out of the Actions
directory.
Running out of memory on CI.
Copy link
Member

@nilsdeppe nilsdeppe left a comment

Choose a reason for hiding this comment

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

The splitting up of the file more seems to have worked. Thanks!

@nilsdeppe nilsdeppe enabled auto-merge February 18, 2025 22:38
@nilsdeppe nilsdeppe added the auto-merge GitHub's auto-merge has been enabled for this PR. label Feb 18, 2025
@nilsdeppe nilsdeppe merged commit 5525b1d into sxs-collaboration:develop Feb 18, 2025
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto-merge GitHub's auto-merge has been enabled for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants