-
Notifications
You must be signed in to change notification settings - Fork 67
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
Parameter linkage bug #858
Conversation
…ar_b" in a link constraint is fixed, if it is a parameter. Updated the readme to show use of the newer interp method for setting values, rather than the deprecated interpolate method, and did the same for the equivalent JOSS example test. Removed the _is_valid_linkage test since it was replaced by a more brief code snipped in _configure_linkages.
…ontrol to verify if they are fixed or not. Noticed that linking ODE outputs (for instance, ke in the case of TestTwoPhaseCannonballODEOutputLinkage.test_traj_param_target_unspecified_units) the linkage report encounters an error if the linked variable is not in the available dictionary. Added a try clause that simply passees if this condition exists, but should address it in the future.
…. This will allow users to better control the optimizers treatment of the resulting constraints.
# For example, see the 'ke' linkage in | ||
# TestTwoPhaseCannonballODEOutputLinkage.test_traj_param_target_unspecified_units. | ||
# For now, let these linkages go undisplayed in the report. | ||
pass |
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.
Should this be "continue" instead of "pass", since the tree_var_a and b are accessed after this line? This also means there is no coverage for this case.
self.assertEqual(str(e.exception), 'Invalid linkage in Trajectory traj: Cannot link final value of "c" in ' | ||
'burn1 to initial value of "c" in coast. Values on both sides of the ' | ||
'linkage are fixed and the linkage is enforced via constraint. Either link ' | ||
'the variables via connection or make the variables design variables on at ' |
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.
I wonder if this is still a little confusing. Maybe say "Either set connected=True or set fix_initial or fix_final to False. I guess it is a matter of telling them what to do in dymos, rather than telling them what to do conceptually. It is up to you though, I don't know how most other people see it.
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.
I went a more conceptual route here because there's more than one way to do it depending on what is being linked. Fundamentally I think the user has to know if their output can be moved by the optimizer on either side of the constraint.
If it's something that has the option to be a design variable, that's a bit easier to deal with, but it can still be a difference between what kind of design variable it is. States and controls have fix_initial=False
and fix_final=False
. States, controls, and parameters have opt=True
that in some cases obviates fix_initial and fix_final. Time has fix_initial
and fix_duration
, and everything also has bounds that could be optionally implemented that could effectively fix their values anyway (lower=0, upper=0).
Summary
Resolves a bug where, when linking a parameter across phases, the check to see if the parameter is fixed (
opt=False
) fails due to a copy/paste error inTrajectory._configure_linkages
.Removed the
_is_valid_linkage
method since it was able to be replaced with fewer lines of code directly in the_configure_linkages
method.Updated the readme and the JOSS test example to reflect the use of
phase.interp
rather thanphase.intepolate
.Added additional arguments to
Trajectory.link_phases
that are passed toTrajectory.add_linkage_constraint
. These arguments allow the user to control how the optimizer treats the resulting contraints.Related Issues
link_phases
method should support arguments ofadd_linkage_constraint
. #855Trajectory._configure_linkages
#857Backwards incompatibilities
None
New Dependencies
None