-
Notifications
You must be signed in to change notification settings - Fork 37
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
Power Regulation Infrastructure #224
Conversation
Job Documentation on 6b38197 wanted to post the following: View the site here This comment will be updated on new commits. |
@gsgall I just saw this PR and notice no one was assigned to review it. Is the PR ready for review, or are you still working on it? |
@csdechant This PR is ready for review! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gsgall Overall, good job with the PR. I did have some questions and recommends on the structure of some of the documentation. I also have some questions on the tests that might result in re-golding some file. One of my bigger questions is did you intended on including an Action for power regulation in this PR, or is that future plans?
test/tests/auxkernels/linear_combination_aux/linear_combination.i
Outdated
Show resolved
Hide resolved
doc/content/source/postprocessors/MultipliedTimeIntegratedPostprocessor.md
Outdated
Show resolved
Hide resolved
doc/content/source/postprocessors/MultipliedTimeIntegratedPostprocessor.md
Outdated
Show resolved
Hide resolved
doc/content/source/postprocessors/PeriodicAmplitudeRegulator.md
Outdated
Show resolved
Hide resolved
doc/content/source/postprocessors/PeriodicAmplitudeRegulator.md
Outdated
Show resolved
Hide resolved
doc/content/source/postprocessors/PeriodicAmplitudeRegulator.md
Outdated
Show resolved
Hide resolved
doc/content/source/postprocessors/PeriodicTimeIntegratedPostprocessor.md
Outdated
Show resolved
Hide resolved
f3360a7
to
02d68ac
Compare
02d68ac
to
31f7c52
Compare
doc/content/source/postprocessors/MultipliedTimeIntegratedPostprocessor.md
Outdated
Show resolved
Hide resolved
doc/content/source/postprocessors/PeriodicTimeIntegratedPostprocessor.md
Outdated
Show resolved
Hide resolved
test/tests/postprocessors/multi_period_averaging/multi_period_averager.i
Outdated
Show resolved
Hide resolved
@gsgall Everything looks good, just very minor things to address. Also, there are a lot of commits here. I would recommend to squashing these commits. |
57f70d7
to
b1e9e44
Compare
d8fa9ff
to
4955400
Compare
@csdechant I squashed the commits I think everything should be good to go! |
Co-authored-by: Corey DeChant <37221357+csdechant@users.noreply.github.com>
@csdechant Is there anything else that needs to be done for this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gsgall Sorry for the delay in reviewing. The only thing that needs to be change is that ## Example Input File Syntax
needs a new line between the header and body text. Everything else looks good.
doc/content/source/postprocessors/MultipliedTimeIntegratedPostprocessor.md
Outdated
Show resolved
Hide resolved
doc/content/source/postprocessors/PeriodicTimeIntegratedPostprocessor.md
Outdated
Show resolved
Hide resolved
Co-authored-by: Corey DeChant <37221357+csdechant@users.noreply.github.com>
@csdechant done! |
closes #222 and enables the users to hold the power deposition into the plasma at a constant value by iteratively updating the applied voltage of a boundary condition
This PR only holds the infrastructure. An issue will be opened to add an action and tutorial for this feature as well to make sure users can easily utilize this since the combination of each of these post processors and auxkernels may not be immediately obvious.