-
Notifications
You must be signed in to change notification settings - Fork 1
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
Update constraint function names for compatibility with ini.requirements #222
Conversation
src/extract/constraints.jl
Outdated
ConstraintFunction(:max_tf_coil_j_margin, "%", dd -> (1. + dd.requirements.coil_j_margin) * dd.build.tf.max_j / dd.build.tf.critical_j, <, 1.0) | ||
ConstraintFunction(:max_oh_coil_j_margin, "%", dd -> (1. + dd.requirements.coil_j_margin) * dd.build.oh.max_j / dd.build.oh.critical_j, <, 1.0) |
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.
These constraints are actually on the maximum tf and oh currents expressed as fraction with respect to the critical currents, including the margins. I think the max_tf_j
and max_oh_j
may be more appropriate
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.
Got rid of margin on both this one and the stresses in the latest commit
src/extract/constraints.jl
Outdated
ConstraintFunction(:max_tf_coil_stress_margin, "%", dd -> (1. + dd.requirements.coil_stress_margin) * maximum(dd.solid_mechanics.center_stack.stress.vonmises.tf) / dd.solid_mechanics.center_stack.properties.yield_strength.tf, <, 1.0) | ||
ConstraintFunction(:max_oh_coil_stress_margin, "%", dd -> (1. + dd.requirements.coil_stress_margin) * maximum(dd.solid_mechanics.center_stack.stress.vonmises.oh) / dd.solid_mechanics.center_stack.properties.yield_strength.oh, <, 1.0) |
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.
same here for stresses
src/extract/constraints.jl
Outdated
ConstraintFunction(:required_flattop_duration, "%", dd -> abs(dd.build.oh.flattop_duration - dd.requirements.flattop_duration) / dd.requirements.flattop_duration, ==, 0.0, 0.01) # relative tolerance | ||
ConstraintFunction(:min_required_flattop_duration, "%", dd -> (dd.build.oh.flattop_duration - dd.requirements.flattop_duration) / dd.requirements.flattop_duration, >, 0.0) |
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.
the logic for having required
in the name is not consistent. Should we add required
everywhere the constraint relates to dd.requirements
, or should we remove it everywhere... since we're talking about a constraint anyways.
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.
Got rid of all the occurrences of "required" in the latest commit!
looks good! @TimSlendebroek does this PR affect any of the example notebooks? |
@orso82 Yes, it'll affect the optimization one - I can update it accordingly! |
Updates the names of the constraint functions to more closely match the corresponding ini.requirements - see here: ProjectTorreyPines/FUSE.jl#792