-
Notifications
You must be signed in to change notification settings - Fork 47
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
Add simple mission benchmarks #46
Add simple mission benchmarks #46
Conversation
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 had a few questions so I didn't approve or request changes yet, whether I have changes to request is TBD based on the answers :)
@@ -36,6 +36,7 @@ | |||
"fix_duration": False, | |||
"initial_bounds": ((0.0, 0.0), "min"), | |||
"duration_bounds": ((64.0, 192.0), "min"), | |||
"add_initial_mass_constraint": False, |
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.
Why are we making this false? If I remember correctly, this is the constraint that ensures the GTOW used in the pre-mission is the same as the initial mass? Seems important...
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.
Close, maybe this could be better named. If it's False then the initial mass is directly connected from pre-mission to mission: https://github.com/OpenMDAO/om-Aviary/blob/main/aviary/interface/methods_for_level2.py#L2409
If True, then we use a constraint enforced at the optimizer level: https://github.com/OpenMDAO/om-Aviary/blob/main/aviary/mission/flops_based/phases/simple_energy_phase.py#L179
Either way the mass is matched up from pre-mission to mission
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.
Ahh gotcha, that's what I get for remembering :)
# TODO: connect this correctly | ||
# mass is the most important to connect but these others should | ||
# be connected as well | ||
# self.model.connect(Mission.Takeoff.FINAL_VELOCITY, | ||
# 'traj.climb.initial_states:mach') | ||
# self.model.connect(Mission.Takeoff.FINAL_ALTITUDE, | ||
# 'traj.climb.controls:altitude') |
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.
Looks like for this case we are losing those initial connections, why did we do that?
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.
These connections remain for the FLOPS based mission. I've added commented out code here for the simple mission to show that we need to connect it later.
We cannot do these connections now because the simplified takeoff outputs velocity whereas the simple mission deals with Mach. We cannot do a direct connection but could do a constraint. Instead, I will make the simplified takeoff output Mach as well and connect that directly in a follow-on PR.
We do not lose any capability the changes in this PR; this is more of a note for the future.
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.
Understood, thanks for the explanation!
Summary
In preparation for combining the
simple
andFLOPS
mission methods into oneheight_energy
method, I've confirmed that the FLOPS swap benchmarks can be run insimple
mission mode with these changes.This PR adds two benchmarks that run the
simple
mission and checks against the same numbers as theFLOPS
mission benchmarks.I'll continue down this path and try to part out the changes into small PRs like this one.
Related Issues
Backwards incompatibilities
None
New Dependencies
None