Unify controls and polynomial controls #1078
Conversation
dymos/phase/phase.py
Outdated
|
|
||
| def add_control(self, name, units=_unspecified, desc=_unspecified, opt=_unspecified, | ||
| fix_initial=_unspecified, fix_final=_unspecified, targets=_unspecified, | ||
| def add_control(self, name, control_type=_unspecified, order=_unspecified, units=_unspecified, desc=_unspecified, |
There was a problem hiding this comment.
We should put control_type and order at the end of the arguments list here. Anyone using positional arguments would have their code broken by this change.
We should also raise deprecation warnings for add_polynomial_control
There was a problem hiding this comment.
Good call, I've reordered control_type to be at the end everywhere.
I have a deprecation warning for add_polynomial_control like this, let me know if you want to see a change:
om.issue_warning(f'{self.pathname}: The method `add_polynomial_control` is '
'deprecated and will be removed in Dymos 2.1. Please use '
'`add_control` with the appropriate options to define a polynomial control.',
category=om.OMDeprecationWarning)
dymos/phase/phase.py
Outdated
| @@ -639,8 +631,8 @@ def add_control(self, name, units=_unspecified, desc=_unspecified, opt=_unspecif | |||
| rate2_continuity_scaler=rate2_continuity_scaler, | |||
| rate2_continuity_ref=rate2_continuity_ref) | |||
|
|
|||
| def set_control_options(self, name, units=_unspecified, desc=_unspecified, opt=_unspecified, | |||
| fix_initial=_unspecified, fix_final=_unspecified, targets=_unspecified, | |||
| def set_control_options(self, name, control_type=_unspecified, order=_unspecified, units=_unspecified, desc=_unspecified, | |||
There was a problem hiding this comment.
Same as above, move control_type and order to the end, and deprecate set_polynomial_control_options.
There was a problem hiding this comment.
I've added it back in and added a deprecation warning.
docs/dymos_book/api/phase_api.ipynb
Outdated
| @@ -220,12 +220,6 @@ | |||
| " :noindex:\n", | |||
| "```\n", | |||
| "\n", | |||
| "## set_polynomial_control_options\n", | |||
There was a problem hiding this comment.
Let's leave this in as deprecated. Also search the ipynb files in docs and make sure that we use the new API in all of the documented examples.
There was a problem hiding this comment.
Got it, I've added that back in and searched through the docs and switched to using the new API; checking CI results now.
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
dymos/phase/phase.py
Outdated
| @@ -619,6 +619,15 @@ def add_control(self, name, order=_unspecified, units=_unspecified, desc=_unspec | |||
| self.control_options[name] = ControlOptionsDictionary() | |||
| self.control_options[name]['name'] = name | |||
|
|
|||
| # if continuity, rate_continuity, or rate2_continuity are not specified, default to False for cases when control_type is 'polynomial' | |||
| if control_type == 'polynomial': | |||
| if continuity is _unspecified: | |||
There was a problem hiding this comment.
There's no such thing as continuity here anyway right? To specify it is wrong. I think you could cover the polynomial case more concisely with continuity = rate_continuity = rate2_continuity = False if the control_type == 'poylnomial'
There was a problem hiding this comment.
Got it, updated accordingly!
Summary
This PR simplifies a lot of the controls-based logic to remove the separation between polynomial and full controls.
This doesn't add or remove any capability but it simplifies a lot of code under the hood.
We'll be able to more easily add other control types (b-splines? others?) if desired with these changes.
All tests are passing locally for me but I'll address any failures on CI as well.
Questions:
control_typeoption that hasfullorpolynomialas possibilities.Backwards incompatibilities
Introduced a deprecation warning for
add_polynomial_control; the method still works in this version but can be removed later on.New Dependencies
None