Unify subsystem addition and solver control across all of our ODEs. - #1284
Unify subsystem addition and solver control across all of our ODEs.#1284Kenneth-T-Moore wants to merge 12 commits into
Conversation
jkirk5
left a comment
There was a problem hiding this comment.
This update is a good demonstration of how OM4 could be valuable for Aviary and a point in favor of updating
| thrust balance. | ||
| couple_aero : bool | ||
| When True, the ODE couples with any aerodynamics subsystems via a force balance. | ||
| aero_solver_sub : None or om.Group |
There was a problem hiding this comment.
This argument seems hyper-specialized for a specific ODE, which I think disqualifies it as useful to include in BaseODE. Is a better approach to have these ODEs have their own definitions for the method? I don't want to keep having to add to BaseODE as we add additional equations of motion that also have special rules for how they want things set up.
There was a problem hiding this comment.
The solved 2dof is the only ODE that creates two different solver groups.
| ) | ||
|
|
||
| return use_mission_solver | ||
| return solver_sub if solver_sub else self |
There was a problem hiding this comment.
Is returning the group useful? I feel like it will cause confusion, because an easy mistaken interpretation of this function is that it returns the correctly configured group of subsystems and solvers they then need to add to the ODE.
If users need the group object for some reason it already got added to the ODE so they should probably use OM interface to access that info in their ODE.
There was a problem hiding this comment.
My suggestion is simply no return statement at all here
There was a problem hiding this comment.
After we return it, some of the ODEs will add the ode or a balance comp to the solver group. I could modify it back so that each ODE is responsible for creating the Group and passing it into the add_subsystems, which is what it was doing originally. It is just some duplicated code.
There was a problem hiding this comment.
Oh ok that makes sense! I guess it is better how you have it here
| from aviary.subsystems.propulsion.propulsion_builder import PropulsionBuilder | ||
| from aviary.subsystems.aerodynamics.aerodynamics_builder import AerodynamicsBuilder |
There was a problem hiding this comment.
I wonder what causes the circular import?? If it is due to imports required for type hints in some other file, I found a way around that (see subsystem_builder.py imports)
There was a problem hiding this comment.
Looks like I'm not seeing any interaction now, so I think I can safely move these back to the top.
Summary
This PR unifies subsystem addition and solver control across all of Aviary's ODEs. Previously, only the Energy method allowed the subsystem to specify that it needed to be in a solver loop.
add_subsystemstoadd_subsystems_and_solver. This method is now also responsible for adding all subsystems to either the top ODE or a solver sub group.needs_mission_solvernow returns False. This should be set to True by the user if any additional subsystem coupling is introduced.add_subsystems_and_solversupports passing in two premade subsystems, and it assigns them to the appropriate solver group.flight_path_ode, which seemed to be an early unfinished experiment in unifying the 2dof ODEs.Note: the solver setup is still not perfect. OpenMDAO 4 promises a complete decoupling of solver specification from the group, and this will streamline most of the difficulties.
Related Issues
Backwards incompatibilities
None
AI Usage
Disclose any AI usage in this PR, including models used and files affected.