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

Constraint aliasing #743

Merged
merged 28 commits into from
May 3, 2022
Merged

Constraint aliasing #743

merged 28 commits into from
May 3, 2022

Conversation

robfalck
Copy link
Contributor

@robfalck robfalck commented Apr 25, 2022

Summary

Implementation of Dymos without separate pass-thru components for path and boundary constraints.

Boundary and Path constraints are all nonlinear by default. This works around an issue with the way OpenMDAO and pyOptSparse currently interact with linear constraints. Users may override the linearity of a constraint by passing linear=True to add_boundary_constraint or add_path_constraint, but if the constrained output is actually a nonlinear function of the design variables this will result in a failure of the optimization.

Adding multiple initial, final, or path constraints to the same index of a variable will now result in an exception. If possible, this condition is detected in add_boundary_constraint or add_path_constraint. If different but equivalent indices are specified on the constraint then this situation is detected during configure.

Outputs which are non-scalar can now have the constrained elements passed by slice objects.

Related Issues

Backwards incompatibilities

In certain cases, treating a constraint which had been marked linear as a nonlinear constraint may pose convergence issues. Please provide feedback if you encounter such cases.

New Dependencies

None

robfalck added 17 commits March 4, 2022 16:01
Dymos now raises an error if a parameter is subject to more than one boundary or path constraint.
…undary constraints, but getting closer to being like numpy
The following tests failed:
test_cannonball_matrix_state.py:TestCannonballMatrixStateExplicitShape.test_cannonball_matrix_state_gl
test_cannonball_matrix_state.py:TestCannonballMatrixStateExplicitShape.test_cannonball_matrix_state_gl_solve_segments
test_cannonball_matrix_state.py:TestCannonballMatrixStateExplicitShape.test_cannonball_matrix_state_radau
test_check_partials.py:TestCheckPartials.test_check_partials_no
test_check_partials.py:TestCheckPartials.test_check_partials_yes
test_explicit_shooting.py:TestExplicitShooting.test_brachistochrone_explicit_shooting
test_explicit_shooting.py:TestExplicitShooting.test_brachistochrone_explicit_shooting_path_constraint
test_explicit_shooting.py:TestExplicitShooting.test_brachistochrone_explicit_shooting_path_constraint_invalid_renamed
test_explicit_shooting.py:TestExplicitShooting.test_brachistochrone_explicit_shooting_path_constraint_polynomial_control
test_explicit_shooting.py:TestExplicitShooting.test_brachistochrone_explicit_shooting_path_constraint_renamed
test_explicit_shooting.py:TestExplicitShooting.test_brachistochrone_explicit_shooting_polynomial_control
test_explicit_shooting.py:TestExplicitShooting.test_brachistochrone_static_gravity_explicit_shooting
test_explicit_shooting.py:TestExplicitShooting.test_explicit_shooting_timeseries_ode_output
test_jupyter_output_linting.py:LintJupyterOutputsTestCase.test_header
test_jupyter_output_linting.py:LintJupyterOutputsTestCase.test_output
test_lint_docstrings.py:DocstringLintTestCase.test_docstrings
test_multi_phase_restart.py:TestExampleTwoBurnOrbitRaiseConnected.test_ex_two_burn_orbit_raise_connected
test_phase.py:TestPhaseBase.test_parameter_final_boundary_constraint
test_phase.py:TestPhaseBase.test_parameter_initial_boundary_constraint
test_phase.py:TestPhaseBase.test_parameter_path_constraint
test_run_problem.py:TestRunProblem.test_run_brachistochrone_problem_refine_case_driver_case_prefix
test_run_problem.py:TestRunProblem.test_run_brachistochrone_problem_refine_case_prefix
test_time_targets.py:TestPhaseTimeTargets.test_explicit_shooting
Allow user to override linear setting of a constraint.  Constraints may always be nonlinear.
Those that introspection finds definitely may not be linear will result in an exception if set to linear.
test_refine_with_shaped_parameter.py:TestRefineShapedStaticParam.test_refine_shaped_static_param_gl
test_refine_with_shaped_parameter.py:TestRefineShapedStaticParam.test_refine_shaped_static_param_radau
test_two_burn_orbit_raise_linkages.py:TestTwoBurnOrbitRaiseLinkages.test_two_burn_orbit_raise_link_control_to_param
…e treated as linear is complex and requires a lot of instrospection code.

Combined with recent issues with linear constraints discovered with the interface between OpenMDAO and pyoptsparse, this makes it far simpler
and less error-prone to simply make all boundary and path constraints nonlinear unless overridden by the user.

TBD: Add some documentation about constraint aliases and the linearity of constraints before merging this one in.
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@coveralls
Copy link

coveralls commented Apr 26, 2022

Coverage Status

Coverage decreased (-0.1%) to 95.562% when pulling c6f87d8 on robfalck:constraint_aliasing into f5aa3cb on OpenMDAO:master.

Internally dymos now converts all constraint indices to their equivalent positive values to make this detection more consistent.
bc_list = self._initial_boundary_constraints if loc == 'initial' else self._final_boundary_constraints

existing_bc = [bc for bc in bc_list
if bc['name'] == name and bc['indices'] == indices and bc['flat_indices'] == flat_indices]
Copy link
Member

Choose a reason for hiding this comment

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

Does bc['indices'] == indices work if they're both ndarrays of equal size? I think that will give ValueError: The truth value of an array with more than one element is ambiguous. Use a.any() or a.all()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. Changed this to catch the common case where the names are the same and indices are both None.
More complicated cases involving overlap of specific indices are worked out during configure.


all_flat_idxs = set()

for con in cons:
Copy link
Member

Choose a reason for hiding this comment

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

Could avoid creating lists you don't need by just setting cons equal to self._initial_boundary_constraints, self._final_boundary_constraints, or self._path_constraints and then in your main loop here just continue if con['name'] == name.

all_flat_idxs = set()

for con in cons:
flat_idxs = set(get_constraint_flat_idxs(con).tolist())
Copy link
Member

Choose a reason for hiding this comment

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

Since you're passing flat_idxs to intersection it doesn't have to be set itself, just an iterator. So if get_constraint_flat_idxs(con) returns a flat array, you could probably pass that directly to intersection.

src_idxs = np.arange(-size, 0, dtype=int).reshape(shape)
# This is a path constraint.
# Remove any flat indices involved in an initial constraint from the path constraint
idxs_not_in_initial = list(set(flat_idxs.tolist()) - idxs_in_initial)
Copy link
Member

Choose a reason for hiding this comment

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

Not sure how big flat_idxs can get, but you compute list(set(flat_idxs.tolist()) twice. Also if flat_idxs is already a flat array you probably don't need the .tolist() either.

The flat indices of a constraint at a single point in time.
"""
if con['indices'] is None:
flat_idxs = np.arange(np.prod(con['shape'], dtype=int), dtype=int).ravel()
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't seem like we need a call to .ravel() here since the array should already be flat.

else:
flat_idxs = indexer(con['indices'], src_shape=con['shape'], flat_src=con['flat_indices']).as_array()

# Convert any negative indices to positive indices, to make detecting overlaps easier.
Copy link
Member

Choose a reason for hiding this comment

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

All of this code to get rid of negative indices is unnecessary if you call .shaped_array() instead of .as_array() above.


phase.add_parameter('g', targets=['g'], units='m/s**2', opt=True)

# Now mistakenly add a boundary constraint at the same loc, variable, and different but equivalent_negative indices
Copy link
Member

Choose a reason for hiding this comment

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

Comment here doesn't reflect the test.

expected = ("In phase traj0.phases.phase0, parameter `g` is subject to multiple boundary or path constraints.\n"
"Parameters are single values that do not change in time, and may only be used in a single "
"boundary or path constraint.")

Copy link
Member

Choose a reason for hiding this comment

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

This error message isn't part of this PR, but it doesn't seem right. Parameters aren't just single values since they can be dynamic (and I think this one is because static_target defaults to False).

Also, is there any scenario in which a parameter can have a boundary and path constraint and still be valid? I don't think it is tested.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Parameters are not dynamic, so you could argue that putting a path constraint on them makes no sense.

When we say that a parameter has static or dynamic targets, all that controls is whether or not the value of the parameter gets fanned out to all nodes in the connection. The value through the phase will not change either way. Allowing it to connect to a target shaped with num_nodes just allows us to connect a parameter or a control to the same target without having to change its shape in the ODE.

Dymos allows users to path constraint them and boundary constraint them, but doing both makes little sense and is prohibited. The last test in test_duplicate_constraints.py tests for the presence of a path and boundary constraint on the same parameter. There's no case in which they could possibly have a different value.

phase.add_objective('time', loc='final', scaler=10)

phase.add_boundary_constraint('y', loc='final', equals=5)
phase.add_path_constraint('y', lower=5, upper=100)
Copy link
Member

Choose a reason for hiding this comment

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

What happens if the path_constraint and boundary_constraint have different vals?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We still only get one constraint per index of a variable, since OpenMDAO will error on multiple constraints applied to different indices.

In Dymos this is resolved by removing the path constraint from the first or last index if there is an initial or final boundary constraint in place. The boundary constraint takes precedence.


transcription.configure_boundary_constraints('final', self)
transcription._configure_boundary_constraints(self)
Copy link
Member

Choose a reason for hiding this comment

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

Kind of wondering why this is the only configure-stack method that has an underscore.

Copy link
Contributor Author

@robfalck robfalck May 2, 2022

Choose a reason for hiding this comment

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

I'm trying to be more cognizant of those methods and members which should be underscored. There's no reason for the user to call the configure methods so they should be underscored, I just haven't moved them all that way yet.

# It's a static parameter, so it shouldn't matter whether we enforce it
# at the start or the end of the phase, so here we'll do both.
# Note if we make these equality constraints, some optimizers (SLSQP) will
# see the problem as infeasible.
Copy link
Member

Choose a reason for hiding this comment

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

Do we do both? I only see initial on 'g'.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, we tested each individually, not both simultaneously.


# We'll let g vary, but make sure it hits the desired value.
# It's a static parameter, so it shouldn't matter whether we enforce it
# at the start or the end of the phase, so here we'll do both.
Copy link
Member

Choose a reason for hiding this comment

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

Same here, maybe you meant each, and not both.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See above

# We'll let g vary, but make sure it hits the desired value.
# It's a static parameter, so it shouldn't matter whether we enforce it
# at the start or the end of the phase. In this case we'll use a path
# constraint to enforce it, but it will still just be a single scalar constraint.
Copy link
Member

Choose a reason for hiding this comment

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

Not sure I understand that this is a scalar constraint. Isn't static_target false?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

static_target=False means that, in the ODE, g is capable of accepting either a control or a parameter. This allows us to use the ODE in different situations without needing to change the shape of g in the ODE itself.

Parameters themselves are always a single value applied throughout the entire phase ("fanned" out to each node), as opposed to a control which can adopt a different value at each node.

@robfalck robfalck merged commit 8d5a9ff into OpenMDAO:master May 3, 2022
@robfalck robfalck deleted the constraint_aliasing branch August 25, 2022 16:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants