Remove FLOPS based mission method #53

johnjasa · 2024-01-09T18:56:45Z

Summary

I've modified all run scripts, tests, benchmarks, and docs to use the simple mission method and removed the FLOPS mission method.
This is probably the crux of my work this jog.
This PR dramatically simplifies the FLOPS-based mission logic and run scripts.

Related Issues

Resolves #

Backwards incompatibilities

None

New Dependencies

None

Kenneth-T-Moore · 2024-01-09T20:59:34Z

I am a little nervous about completely removing the old method. We might want it to be able to build off of legacy results using the new aviary, or we might want to do some testing with it.

johnjasa · 2024-01-09T21:24:24Z

I am a little nervous about completely removing the old method. We might want it to be able to build off of legacy results using the new aviary, or we might want to do some testing with it.

Can you tell me more about the use cases you'd want to keep it around for? The underlying physics of the simple height energy equations of motion are the same as in FLOPS. Would you ever want new users to use the older mission method or just keep it around for comparisons?

Kenneth-T-Moore · 2024-01-10T17:31:15Z

I am a little nervous about completely removing the old method. We might want it to be able to build off of legacy results using the new aviary, or we might want to do some testing with it.

Can you tell me more about the use cases you'd want to keep it around for? The underlying physics of the simple height energy equations of motion are the same as in FLOPS. Would you ever want new users to use the older mission method or just keep it around for comparisons?

I am mostly thinking in terms of the MBSAE published work, and being able to reproduce it exactly as we migrate all of that to Aviary 0.9.x.

johnjasa · 2024-01-10T21:13:26Z

Ken and I had a great conversation about this and established this path forward:

Irian will convert the MBSA&E models to use Aviary 0.9.1+
Ken and John will help convert this to the simple mission
This PR can come in and when it does and with all added features we should increment to Aviary 0.9.2

jkirk5

Good stuff! I have a ton of little nitpicks about some changes, especially instances where simple was being brought back to name or label things.

jkirk5 · 2024-01-17T20:11:10Z

aviary/docs/getting_started/onboarding_level1.ipynb

Why change height_energy to simple? And phase info paths are wrong. I guess this whole file needs to get updated, seems like it is very out-of-date on how to set up mission & phase info. If you don't want to deal with it for this PR we can make it a task for someone to tackle.

Thanks for your comments, Jason -- anything where simple snuck back in was just an artifact of this branch being out-of-date with main. I was waiting for Ken's assessment for MBSA&E work. I've worked through the merge conflicts now; I'll update any remaining notions of simple as you pointed out.

jkirk5 · 2024-01-17T20:17:06Z

aviary/interface/methods_for_level2.py

@@ -694,7 +691,7 @@ def _add_groundroll_eq_constraint(self, phase):
            promotes_inputs=["t_init_gear", "t_init_flaps"],
        )

-    def _get_height_energy_phase(self, phase_name, phase_idx):
+    def _get_simple_phase(self, phase_name, phase_idx):


Why change from height_energy to simple?

jkirk5 · 2024-01-17T20:18:36Z

aviary/interface/methods_for_level2.py

            for phase_idx, phase_name in enumerate(phases):
                phase = traj.add_phase(
-                    phase_name, self._get_height_energy_phase(phase_name, phase_idx))
+                    phase_name, self._get_simple_phase(phase_name, phase_idx))


same question here, why bring back simple

jkirk5 · 2024-01-17T20:19:59Z

aviary/interface/test/test_check_phase_info.py

@@ -11,7 +11,7 @@


 class TestCheckInputs(unittest.TestCase):
-    def test_correct_input_flops(self):
+    def test_correct_input_simple(self):


More simple! FLOPS -> height_energy

jkirk5 · 2024-01-17T20:20:54Z

aviary/interface/test/test_check_phase_info.py

@@ -11,7 +11,7 @@


 class TestCheckInputs(unittest.TestCase):
-    def test_correct_input_flops(self):
+    def test_correct_input_simple(self):
        # This should pass without any issue as it's the same valid dict as before
        self.assertTrue(check_phase_info(phase_info_flops, mission_method=HEIGHT_ENERGY))


flops & gasp in this file should get changed to height_energy and 2dof

jkirk5 · 2024-01-17T20:26:34Z

aviary/models/test_aircraft/aircraft_for_bench_FwFm_simple.csv

You can delete this file - it should be an exact copy of FwFm.csv now

jkirk5 · 2024-01-17T20:29:29Z

aviary/validation_cases/benchmark_tests/test_0_iters.py

-    def test_height_energy_zero_iters(self):
-        local_phase_info = deepcopy(height_energy_phase_info)
-        self.build_and_run_problem('models/test_aircraft/aircraft_for_bench_FwFm.csv',
+    def test_simple_zero_iters(self):


jkirk5 · 2024-01-17T20:29:54Z

aviary/validation_cases/benchmark_tests/test_0_iters.py

@@ -5,7 +5,7 @@

 from aviary.interface.methods_for_level2 import AviaryProblem
 from aviary.interface.default_phase_info.two_dof import phase_info as two_dof_phase_info
-from aviary.interface.default_phase_info.height_energy import phase_info as height_energy_phase_info
+from aviary.interface.default_phase_info.height_energy import phase_info as simple_phase_info


more height_energy reverted to simple

jkirk5 · 2024-01-17T20:32:22Z

aviary/validation_cases/benchmark_tests/test_subsystems_within_a_mission.py

-                'subsystem_options': {
-                    'core_aerodynamics': {'method': 'solved_alpha', 'aero_data': aero_data, 'training_data': False}
-                },
+                "subsystem_options": {"core_aerodynamics": {"method": "computed"}},


Why was aero method changed? It should be mission agnostic (and if it is somehow not, then we need a bug report!)

Changed for no particular reason; I copied-pasted this phase_info from another place. It is indeed mission agnostic

jkirk5 · 2024-01-17T20:33:24Z

aviary/variable_info/enums.py

    TWO_DEGREES_OF_FREEDOM = '2DOF'
    # TODO these are a little out of place atm
-    SIMPLE = 'simple'
+    HEIGHT_ENERGY = 'height_energy'


You made them non-alphabetical! Plus the TODO is implied to apply to height_energy now, but that one's fine

order moved and comment removed

jkirk5 · 2024-01-17T20:39:58Z

I am mostly thinking in terms of the MBSAE published work, and being able to reproduce it exactly as we migrate all of that to Aviary 0.9.x.

I don't think its reasonable to always expect identical results moving to a new version of Aviary, especially as the code is rapidly evolving. Long term best practice will be archiving the environment used to run final results in case replication is needed.

Kenneth-T-Moore · 2024-01-19T14:13:55Z

aviary/validation_cases/benchmark_tests/test_bench_FwFm.py

            },
        }

        prob = run_aviary(
-            'models/test_aircraft/aircraft_for_bench_FwFm_simple.csv', phase_info)
+            'models/test_aircraft/aircraft_for_bench_FwFm.csv', phase_info, max_iter=50)


Are we hitting maxiter on this benchmark? It is the FLOPS/FLOPS test, so I would expect it to converge more quickly.

Not hitting max_iter; I added it during debugging

Kenneth-T-Moore · 2024-01-19T14:32:06Z

aviary/validation_cases/benchmark_tests/test_FLOPS_based_sizing_N3CC.py


    traj = setup_trajectory_params(prob.model, traj, aviary_inputs)

    ##################################
    # Connect in Takeoff and Landing #
    ##################################
-
-    prob.model.add_subsystem(


It is interesting that this benchmark used slack variables instead of a connection for the takeoff to climb connection. (even more interesting since I wrote this benchmark, but I must have copied from a different benchmark that is long gone.)

I guess that removing the slack variables makes it more like how we currently do it in level 2.

Kenneth-T-Moore · 2024-01-19T14:38:11Z

aviary/validation_cases/benchmark_tests/test_0_iters.py

@@ -63,6 +63,4 @@ def test_zero_iters_solved(self):




Please keep these.

Note: testflo doesn't require unittest.main(). We only need that if we run with unittest, or if we want the test for run with python test_0_iters.py. You can't debug through unittest though, which is why I like to keep the other lines.

Changed back!

Kenneth-T-Moore · 2024-01-19T14:40:41Z

aviary/subsystems/propulsion/test/test_custom_engine_model.py

        prob = AviaryProblem(reports=False)

        # Load aircraft and options data from user
        # Allow for user overrides here
-        prob.load_inputs(csv_path, phase_info, engine_builder=SimpleTestEngine())
+        prob.load_inputs("models/test_aircraft/aircraft_for_bench_GwFm.csv",
+                         phase_info, engine_builder=SimpleTestEngine())


 # Preprocess inputs


indentation

thanks, fixed

Kenneth-T-Moore · 2024-01-19T14:41:20Z

aviary/subsystems/propulsion/test/test_custom_engine_model.py

-        for var_name in expected_var_names:
-            self.assertIn(var_name, design_vars_dict)
-
-        # Check values


Why were these asserts all removed?

Not important for testing if the custom engine model was used correctly; I had to update their names and realized there was no reason for them to be there

Kenneth-T-Moore · 2024-01-19T14:41:38Z

aviary/subsystems/aerodynamics/flops_based/test/test_solved_aero_group.py

@@ -279,6 +349,4 @@ def build_pre_mission(self, aviary_inputs):


 if __name__ == "__main__":
-    # unittest.main()


please restore these.

Kenneth-T-Moore · 2024-01-19T14:42:43Z

aviary/subsystems/aerodynamics/flops_based/test/test_solved_aero_group.py

+        assert_near_equal(CL_pass, CL_base, 1e-6)
+        assert_near_equal(CD_pass, CD_base, 1e-6)
+
+    def test_solved_aero_pass_polar_unique_abscissa(self):


Was this test deleted in a regression, or was it moved from elsewhere? I remember writing this, but it appears as new in this diff.

Great catch, it was unnecessarily duplicated in this PR, I fixed that now

Kenneth-T-Moore · 2024-01-19T15:15:13Z

aviary/mission/flops_based/ode/mission_ODE.py

@@ -152,3 +196,12 @@ def setup(self):
                               ('mass', Dynamic.Mission.MASS)
                           ],
                           promotes_outputs=['initial_mass_residual'])
+
+        self.nonlinear_solver = om.NewtonSolver(solve_subsystems=True,


Adding the solver has some ramifications. I wondered if you could confine it to a subsystem, but looks like that subsystem would need to contain at least the engine, and that's in the external subsystem group, so there probably isn't anything else we can do.

I agree about the ramifications and lack of potential simplification with the current setup. Is there something you'd like to see different here in the PR or are you simply noting this consideration?

Kenneth-T-Moore · 2024-01-19T15:21:33Z

aviary/interface/test/test_phase_info.py

@@ -125,46 +121,6 @@ def test_phase_info_parameterization_gasp(self):
        assert_near_equal(prob.get_val("traj.cruise.rhs.mach")[0],
                          0.6)

-    def test_phase_info_parameterization_flops(self):


Rather than deleting this test, I think the right thing would be to convert the parameterizing method from the old height_energy phase info so that it works on the new one. This was added to support the level 1 command line interface, when you run it on an aircraft that is not the single-aisle-transport.

Understood, Ken, thanks for explaining! I've updated this and pushed up.

Kenneth-T-Moore · 2024-01-19T15:23:53Z

aviary/interface/default_phase_info/height_energy.py

    },
 }
-
-
-def phase_info_parameterization(phase_info, aviary_inputs):


As in earlier comment, this needs to be migrated to not lose a command line feature that was essential to the release.

Agreed, added back now.

Kenneth-T-Moore · 2024-01-19T15:27:59Z

aviary/interface/methods_for_level2.py

-                # self.model.connect(Mission.Takeoff.FINAL_ALTITUDE,
-                #                 'traj.climb.controls:altitude')
+
+            # Create an ExecComp to compute the difference in mach


Oh, we do have a slack variable! (Mostly because mach isn't a state any more.)
I wonder if we should name the component more descriptively -- mach_resid_takeoff_cruise or maybe you have some other ideas?

Same with altitude.

Went with mach_resid_for_connecting_takeoff and altitude_resid_for_connecting_takeoff, thanks!

Kenneth-T-Moore · 2024-01-19T15:30:37Z

aviary/interface/methods_for_level2.py

-            self.model.connect('traj.descent.control_values:altitude', Mission.Landing.INITIAL_ALTITUDE,
-                               src_indices=[0])
+        self.model.connect('traj.descent.control_values:altitude', Mission.Landing.INITIAL_ALTITUDE,
+                           src_indices=[0])


Can you take a look at an n2 of one of the FLOPS models and make sure that none of the component changes are producing any cycles (i.e., everything is in the correct order)?

I've done this and confirmed that the only cycle introduced is the solver-based throttle loop, as expected. Thanks!

Kenneth-T-Moore

A few comments to address.

johnjasa · 2024-01-19T17:52:05Z

Thanks for these comments, @Kenneth-T-Moore! Please let me know if you'd like to see any other changes. I'm working on the docs failures now.

johnjasa added 8 commits January 8, 2024 17:31

Converting over tests to not use FLOPS mission method

24713b6

updating more tests

c91210b

All tests pass, benchmarks next

0005f91

All benchmarks pass with simple mission

f3ad946

All tests pass; benchmarks next

a7b0e50

Removed old FLOPS based L3 missions

46b74bf

Updating docs

e0af92b

merging

d766dd2

johnjasa changed the title ~~Removed FLOPS based mission method~~ Remove FLOPS based mission method Jan 9, 2024

johnjasa added 12 commits January 10, 2024 15:13

All benches pass

5e64a19

Merging

409cbf9

All tests, docs, and benches pass

c9f956a

Moving to height_energy

68fba1b

Added missing file

2fa1bc3

Moved simple to mission

0c9be9b

Fixed doc build

3169b39

Merging

d1ba030

Tests passing

886b93d

Filter out Dymos solver warnings

8b61a46

Tests and benches pass

4dfbc2b

Removed defunct variable in takeoff

bf53319

jkirk5 requested changes Jan 17, 2024

View reviewed changes

johnjasa added 3 commits January 18, 2024 16:17

WIP; merging

cb01c44

updating benchmarks

f896008

Updated tests and docs

be4f91e

johnjasa added 2 commits January 18, 2024 18:27

Updated tests and docs

2674c0c

Feedback based on PR

6c0c337

Kenneth-T-Moore reviewed Jan 19, 2024

View reviewed changes

jkirk5 approved these changes Jan 19, 2024

View reviewed changes

Kenneth-T-Moore reviewed Jan 19, 2024

View reviewed changes

Kenneth-T-Moore requested changes Jan 19, 2024

View reviewed changes

PR feedback changes

fc1e4c2

Kenneth-T-Moore approved these changes Jan 19, 2024

View reviewed changes

Fixed doc build

7f9dc5b

johnjasa enabled auto-merge January 19, 2024 17:59

johnjasa added this pull request to the merge queue Jan 19, 2024

Merged via the queue into OpenMDAO:main with commit 4f0b3e7 Jan 19, 2024
6 checks passed

crecine mentioned this pull request Jan 19, 2024

Make GASP-based phases use phase builders #88

Merged

		@@ -279,6 +349,4 @@ def build_pre_mission(self, aviary_inputs):


		if __name__ == "__main__":
		# unittest.main()

Remove FLOPS based mission method #53

Remove FLOPS based mission method #53

Conversation

johnjasa commented Jan 9, 2024 • edited

Summary

Related Issues

Backwards incompatibilities

New Dependencies

Kenneth-T-Moore commented Jan 9, 2024

johnjasa commented Jan 9, 2024

Kenneth-T-Moore commented Jan 10, 2024

johnjasa commented Jan 10, 2024 • edited

jkirk5 left a comment

Choose a reason for hiding this comment

jkirk5 Jan 17, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkirk5 Jan 17, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkirk5 commented Jan 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kenneth-T-Moore Jan 19, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kenneth-T-Moore left a comment

Choose a reason for hiding this comment

johnjasa commented Jan 19, 2024

johnjasa commented Jan 9, 2024 •

edited

johnjasa commented Jan 10, 2024 •

edited

jkirk5 Jan 17, 2024 •

edited

jkirk5 Jan 17, 2024 •

edited

Kenneth-T-Moore Jan 19, 2024 •

edited