-
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 a FunctionComp example #710
Conversation
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
docs/dymos_book/examples/balanced_field/balanced_field_funccomp.ipynb
Outdated
Show resolved
Hide resolved
"traj.add_phase('rotate', rotate)\n", | ||
"traj.add_phase('climb', climb)\n", | ||
"\n", | ||
"# Add parameters common to multiple phases to the trajectory\n", |
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.
This is ALOT of paramters. Most of them are things you probably would not change... like mu.
can we use default values for these in the function arguments (hopefully the function comp API picks those up for default values)? Then just promote some of the inputs to demonstrate you can and add a comment that others are left at their default values?
As is this code makes it seem like you must promote everything as prameters.
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 took a pass at removing some of the parameters that didn't change from phase to phase.
"import openmdao.func_api as omf\n", | ||
"\n", | ||
"\n", | ||
"class BalancedFieldODEComp(om.ExplicitFuncComp):\n", |
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 would not sub-class ExplicitFuncComp here. I would make a contructor function that builds the meta object for you, with some arguments like num_nodes, and other switches. Then just pass that meta object to the stock-standard ExplicitFunctionComp.
Hopefully the function-comp api gives you a way to declare an option somehow... and you can use that option when you call the meta constructor inside the phase's setup. The component can have that option, even though non of the compute methods need it. You might still need to pass it in to the function though.
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.
@JustinSGray Implemented as a factory function instead of subclassing. I agree it's cleaner.
Added a factory function to return a copy of the ODE class. This function behaves like a class constructor, so from the dymos perspective it can be used in place of ode_class. Found an issue where Oswald's efficiency factor was 801 instead of 0.801. Cleaned up trajectory parameter settings, leaving them default where possible for brevity.
…~5.6 seconds to ~3.6 seconds
… longer supports 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.
much improved from the first version. The wrap_func_ode
was a good compromise.
Summary
This PR adds an example that is a version of the balanced field problem that uses a first-class function for the ODE and wraps it with ExplicitFuncComp.
jax
is used to compute the derivatives.Related Issues
Backwards incompatibilities
None
New Dependencies
None