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

Added several chain controls #29161

Open
wants to merge 14 commits into
base: next
Choose a base branch
from

Conversation

joshuahansel
Copy link
Contributor

Refs #28948

@moosebuild
Copy link
Contributor

moosebuild commented Nov 28, 2024

Job Documentation, step Docs: sync website on 4ac8a27 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.

looks good


protected:
/// Minimum value to apply to control data
const Real _min_value;
Copy link
Contributor

@GiudGiud GiudGiud Nov 29, 2024

Choose a reason for hiding this comment

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

would be nice to have these be references to Reals from other ChainControls?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would, but I'd prefer that the user not be required to add chain controls for the bounds if they just want constant values. To get the best of both approaches, I think we'd need to do some work to allow constants to be able to be supplied for control data parameters, and I'd prefer to do that work when it becomes needed.

protected:
/// Minimum value to apply to control data
const Real _min_value;
/// Maximum value to apply to control data
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// Maximum value to apply to control data
/// Maximum value to restrict control data

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed wording to "Lower/upper bound to apply to"

47,4.8295863577061
48,4.628367040742
49,4.8920922443363
50,4.6075489928787
Copy link
Contributor

Choose a reason for hiding this comment

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

seen some closer numbers to 5.
does it make it there eventually?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It gets there very slowly - just wanted to show that it's converging. However, I do see that these parameters get there in an oscillatory fashion, so I'm going to change and re-gold.

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