-
Notifications
You must be signed in to change notification settings - Fork 68
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
Simplify ssto #534
Simplify ssto #534
Conversation
elif cb == 'moon': | ||
rho_ref = 0.0 | ||
h_scale = 1.0 | ||
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.
Don't need the else
branch because self.options already does value-checking to assure that "central_body" is only in ['earth', 'moon']
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.
Honestly we're not even running a test against "moon" any more (and haven't been for some time). There's no need to add this complication here, just remove cb
as an option and replace with rho_ref
and h_scale
options which default to the Earth values.
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 test_parameters failure is just due to the targets of the states and parameters changing since there's no longer a nested EOM. Since the variables have the same names as the inputs of the ODE, targets should just be omitted. State rates will need to be changed as well. Units are the same as those defined in the ODE and can also be removed.
elif cb == 'moon': | ||
rho_ref = 0.0 | ||
h_scale = 1.0 | ||
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.
Honestly we're not even running a test against "moon" any more (and haven't been for some time). There's no need to add this complication here, just remove cb
as an option and replace with rho_ref
and h_scale
options which default to the Earth values.
F_T = inputs['thrust'] | ||
Isp = inputs['Isp'] | ||
|
||
g = _g[self.options['central_body']] |
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.
also include g
as an option, since we're only testing against Earth.
self.declare_partials(of='mdot', wrt='Isp', rows=ar, cols=ar) | ||
|
||
# # Complex-step derivatives | ||
# self.declare_partials(of='*', wrt='*', method='cs') |
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.
With this PR (OpenMDAO/OpenMDAO#1862) these lines should work now - can remove the other declare_partials
calls, and removal of compute_partials
@@ -45,18 +43,18 @@ def test_doc_ssto_earth(self): | |||
phase.set_time_options(initial_bounds=(0, 0), duration_bounds=(10, 500)) | |||
|
|||
phase.add_state('x', fix_initial=True, ref=1.0E5, defect_ref=1.0, | |||
rate_source='eom.xdot', units='m') | |||
rate_source='xdot', units='m') |
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.
targets and units here are no longer necessary, since the targets have the same name as the states/controls/parameters.
…it with options for rho_ref and h_scale. Removed targets and units where not longer necessary
updated some docs for kaushiks SSTO case
Summary
Modified SSTO problem to be single component rather than a group. Added code to declare derivatives to use complex step
Related Issues
Status
Backwards incompatibilities
None
New Dependencies
None