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

Small PIDTransientControl enhancement #26743

Merged
merged 4 commits into from
Feb 7, 2024

Conversation

jthano
Copy link
Contributor

@jthano jthano commented Feb 5, 2024

closes #26742

@moosebuild
Copy link
Contributor

moosebuild commented Feb 5, 2024

Job Documentation on 3263f4e wanted to post the following:

View the site here

This comment will be updated on new commits.

Copy link
Contributor

@GiudGiud GiudGiud left a comment

Choose a reason for hiding this comment

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

There's two changes here, but only one is tested

Add to documentation:

How to use in a transient

...

How to use in a steady solve

...
!listing

Add an input for steady solves if you want to enable this

@jthano
Copy link
Contributor Author

jthano commented Feb 5, 2024

There's two changes here, but only one is tested

Add to documentation:

How to use in a transient

...

How to use in a steady solve

... !listing

Add an input for steady solves if you want to enable this

Okay, I'll look into this more. I mentioned Steady executioner in the warning message because the original mooseError message said If using a Steady Executioner, add a minimum number of Picard iterations and comment this error. Do you know what the use case was that inspired the original message? I could just change my mooseWarning to a mooseError and not mention the Steady Executioner?

@GiudGiud
Copy link
Contributor

GiudGiud commented Feb 5, 2024

Do you know what the use case was that inspired the original message? I

the use case is that you can consider doing a picard multiapp iteration with the PIDController active.
Didnt it start as a mooseError? Are you planning to use this with a steady executioner

@jthano
Copy link
Contributor Author

jthano commented Feb 5, 2024

Do you know what the use case was that inspired the original message? I

the use case is that you can consider doing a picard multiapp iteration with the PIDController active. Didnt it start as a mooseError? Are you planning to use this with a steady executioner

No in my use case the problem will be a transient (with a custom Executioner that is not Transient) and I won't see this warning message. I didn't understand the original mooseError message that well. It seems that if you suggest to the user to comment out an error and recompile for their use case, then it should be a warning, as in the usage they want is advanced and non-typical but supported to some extent. I can add a test for using this with a Steady executioner or I can just change my mooseWarning back to a mooseError with a message similar to the original one. I like the idea of trying to add a test with a Steady executioner if the original intention of the object was to support that as an advanced use case.

@GiudGiud
Copy link
Contributor

GiudGiud commented Feb 5, 2024

I did not have time to make sure it worked with Steady so I added an error instead of a warning.
It's not super supported. The P ok, the Integral and the derivative are meh / ill-defined with a steady executioner.

@jthano
Copy link
Contributor Author

jthano commented Feb 5, 2024

I did not have time to make sure it worked with Steady so I added an error instead of a warning. It's not super supported. The P ok, the Integral and the derivative are meh / ill-defined with a steady executioner.

Okay, I'll change it back to a mooseError then. I think that is the best solution.

@moosebuild
Copy link
Contributor

moosebuild commented Feb 5, 2024

Job Coverage on 3263f4e wanted to post the following:

Framework coverage

519d81 #26743 3263f4
Total Total +/- New
Rate 85.21% 85.21% +0.00% 90.91%
Hits 99626 99628 +2 10
Misses 17292 17291 -1 1

Diff coverage report

Full coverage report

Modules coverage

Coverage did not change

Full coverage reports

Reports

This comment will be updated on new commits.

@jthano
Copy link
Contributor Author

jthano commented Feb 6, 2024

I changed the warning to an error. Let me know if you agree with the message. I think if someone tries to use the object with a Steady executioner, they will know based on the error message to open an issue and ask for that to be supported or do it themselves.

Copy link
Contributor

@GiudGiud GiudGiud 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.

@jthano jthano force-pushed the small_pidtrasientcontrol_update branch from 8c2c91e to b341013 Compare February 6, 2024 16:48
@jthano jthano force-pushed the small_pidtrasientcontrol_update branch from b341013 to 3263f4e Compare February 6, 2024 22:27
@GiudGiud GiudGiud merged commit f5fb7ad into idaholab:next Feb 7, 2024
47 checks passed
@jthano jthano deleted the small_pidtrasientcontrol_update branch February 7, 2024 20:20
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PIDTransientControl object enhancement
3 participants