-
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
Added ability to add expressions as constraints #875
Conversation
… is an expression
…its input variables
…es output is an expression. If it is, then the output name option is used instead of the name
…ensured no white spaces are left in variable names
…n the explicit shooting transcription
…apalli/dymos into timeseries_expr
…apalli/dymos into timeseries_expr
…pass keyword arguments to the enclosed ExecComp.
…ik_timeseries_expr
More test and timeseries expression keyword arguments
… avoid mangling absolute shape and relative shape
…into constraint_expr
…n. Added capability for boundary constraints to find timeseries expr variables
|
||
|
||
@use_tempdirs | ||
class TestBrachistochroneStaticGravity(unittest.TestCase): |
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.
Please change this to TestBrachistochroneExprPathConstraint, just to make it a bit more obvious what it's testing.
dymos/examples/brachistochrone/test/test_brachistochrone_path_constraint.py
Outdated
Show resolved
Hide resolved
…matrix multiplication as an oerator to look for
dymos/utils/introspection.py
Outdated
units = output_options['units'] if output_options['units'] is not _unspecified else None | ||
shape = output_options['shape'] if output_options['shape'] not in {_unspecified, None} else (1,) | ||
|
||
abs_names = [x.strip() for x in re.findall(re.compile(r'([_a-zA-Z]\w*[ ]*\(?:?[.]?)'), expr) |
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.
You might want to do the regex compilation once before the beginning of the outer loop instead of here.
p['phase0.parameters:g'] = 9.80665 | ||
return p | ||
|
||
def tearDown(self): |
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.
Probably don't need the Teardown since we are using Tempdirs.
@@ -1083,9 +1083,9 @@ def add_boundary_constraint(self, name, loc, constraint_name=None, units=None, | |||
loc : str |
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 docstring description for "name" should also be modified to explain that it can be an expression of variables.
dymos/phase/phase.py
Outdated
The name of the variable as provided to the boundary constraint comp. By | ||
default this is the last element in `name` when split by dots. The user may | ||
override the constraint name if splitting the path causes name collisions. | ||
The name of the boundary constraint. By default, this is the left-hand side of |
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.
Reword: "By default, this is 'var_constraint' if name is a single variable, or the left-hand side of the equation if name is an expression."
|
||
phase.add_boundary_constraint('x', loc='final', equals=10.0) | ||
phase.add_path_constraint('pc = y-x/2-1', lower=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.
Is the Left Hand Side always required? maybe that info should be presented in the docstring for "name".
raise ValueError(f'The expression provided {name} has invalid format. ' | ||
'Expression may be a single variable or an equation' | ||
'of the form "constraint_name = func(vars)"') | ||
else: |
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.
For coverage, you should include a test for every error message that you added. They can be simple contrived tests that live in the tests folder in the code rather than something complex in the examples.
raise ValueError(f'The expression provided {name} has invalid format. ' | ||
'Expression may be a single variable or an equation' | ||
'of the form "constraint_name = func(vars)"') | ||
else: |
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 question the need for all of this expression checking (here) and the name checking (in introspection). I think OpenMDAO might do all this checking in the execcomp, so these might be redundant. The only reason you might do this here is to deliver a better error message to the user.
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
Summary
Allows user to specify an expression to be used as a path or boundary constraint.
Related Issues
Backwards incompatibilities
None
New Dependencies
None