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

UserWorkBeforeRestartOutput #1062

Merged
merged 24 commits into from
May 2, 2024
Merged

UserWorkBeforeRestartOutput #1062

merged 24 commits into from
May 2, 2024

Conversation

brryan
Copy link
Collaborator

@brryan brryan commented Apr 22, 2024

PR Summary

We have a downstream need to call a third party library's restart capability when writing our own restarts. This PR adds hooks for Mesh and MeshBlock analogs of UserWorkBeforeOutput that are only triggered during restart outputs.

PR Checklist

  • Code passes cpplint
  • New features are documented.
  • Adds a test for any bugs fixed. Adds tests for new features.
  • Code is formatted
  • Changes are summarized in CHANGELOG.md
  • Change is breaking (API, behavior, ...)
    • Change is additionally added to CHANGELOG.md in the breaking section
    • PR is marked as breaking
    • Short summary API changes at the top of the PR (plus optionally with an automated update/fix script)
  • CI has been triggered on Darwin for performance regression tests.
  • Docs build
  • (@lanl.gov employees) Update copyright on changed files

@brryan brryan changed the title WIP: UserWorkRestartOutput WIP: UserWorkBeforeRestartOutput Apr 22, 2024
src/application_input.hpp Outdated Show resolved Hide resolved
@Yurlungur
Copy link
Collaborator

@brryan is this ready for review? It's still marked WIP

@brryan brryan changed the title WIP: UserWorkBeforeRestartOutput UserWorkBeforeRestartOutput Apr 25, 2024
@brryan
Copy link
Collaborator Author

brryan commented Apr 25, 2024

@brryan is this ready for review? It's still marked WIP

@Yurlungur Yes it works but I just wanted to see what people think about including OutputParameters* in the function signature for UserWorkBeforeRestartOutput (and somehow extending UserWorkBeforeOutput in the same way?) so we could get things like output filename downstream.

Copy link
Collaborator

@Yurlungur Yurlungur left a comment

Choose a reason for hiding this comment

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

I am fine with outputting OutputParameters. Also I wonder if this should be in StateDescriptor in addition to or instead of ApplicationInput?

Also should the flag be documented in the docs?

A few comments below. However, consider this non-blocking. I'll approve now.

src/mesh/mesh.cpp Outdated Show resolved Hide resolved
src/pgen/default_pgen.cpp Outdated Show resolved Hide resolved
src/mesh/mesh.cpp Outdated Show resolved Hide resolved
@brryan brryan requested a review from pgrete April 29, 2024 19:07
@brryan brryan requested a review from pdmullen April 29, 2024 19:07
src/outputs/outputs.cpp Show resolved Hide resolved
Copy link
Collaborator

@pgrete pgrete left a comment

Choose a reason for hiding this comment

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

As mentioned on Thursday, I'm happy with forwarding output params (even if its' a breaking change).
With that in mind, I could also imagine a solution to the original problem without introducing new callbacks by just checking on output type internally downstream, but having additional callbacks also probably doesn't hurt.

src/interface/state_descriptor.hpp Outdated Show resolved Hide resolved
src/mesh/mesh.cpp Outdated Show resolved Hide resolved
src/outputs/outputs.cpp Show resolved Hide resolved
@brryan
Copy link
Collaborator Author

brryan commented May 1, 2024

OK I updated this to reintroduce explicitness around nullptr with std::function and I removed the per-MeshBlock version of UserWorkBeforeRestartOutput. What I did not do is:

  • Remove the per-MeshBlock version of UserWorkBeforeOutput
  • Change the signature of UserWorkBeforeOutput to include OutputParameters.

That would pick up two breaking changes so maybe we want to get some consensus on a separate PR that refactors some of these hooks sometime in the future? If so I think this is ready to go, @pgrete if you are OK to approve (and others don't want to pause before merge) I will then go ahead and merge this in.

Copy link
Collaborator

@pgrete pgrete left a comment

Choose a reason for hiding this comment

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

LGTM

src/driver/driver.cpp Show resolved Hide resolved
@brryan brryan enabled auto-merge (squash) May 2, 2024 14:14
@brryan brryan disabled auto-merge May 2, 2024 17:03
@brryan brryan enabled auto-merge (squash) May 2, 2024 18:17
@brryan brryan merged commit c1b43c4 into develop May 2, 2024
49 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.

4 participants