Takeoff segment #473
Takeoff segment #473
Conversation
New values are obtained with 1 meter accuracy on route distance.
…Renamed takeoffspeedchange in ground_speed_change.py
…friction'. Tests working in non isa (T+10C and +1000m) Test 'test_additional_segment_parameter' is set to illustrate the problem of passing class variables declared in segment classes throught the mission file. It works if the variables are declared in the base.py class.
…he mission definition file. GroundSegment introduced with parameter "wheels_friction". Clean-up and correction of tests.
# Conflicts: # docs/documentation/mission_module/mission_file/segments.rst # docs/documentation/mission_module/mission_module.rst # src/fastoad/model_base/flight_point.py # src/fastoad/models/performances/mission/base.py # src/fastoad/models/performances/mission/mission_definition/mission_builder.py # src/fastoad/models/performances/mission/mission_definition/schema.py # src/fastoad/models/performances/mission/mission_definition/tests/data/mission.yml # src/fastoad/models/performances/mission/mission_definition/tests/test_mission_builder.py # src/fastoad/models/performances/mission/mission_definition/tests/test_schema.py # src/fastoad/models/performances/mission/openmdao/mission.py # src/fastoad/models/performances/mission/openmdao/mission_component.py # src/fastoad/models/performances/mission/openmdao/mission_wrapper.py # src/fastoad/models/performances/mission/openmdao/resources/sizing_breguet.yml # src/fastoad/models/performances/mission/openmdao/resources/sizing_mission.yml # src/fastoad/models/performances/mission/openmdao/tests/data/test_breguet.xml # src/fastoad/models/performances/mission/openmdao/tests/data/test_breguet.yml # src/fastoad/models/performances/mission/openmdao/tests/data/test_breguet_from_block_fuel.yml # src/fastoad/models/performances/mission/openmdao/tests/data/test_mission.yml # src/fastoad/models/performances/mission/openmdao/tests/test_mission.py # src/fastoad/models/performances/mission/openmdao/tests/test_sizing_mission.py # src/fastoad/models/performances/mission/routes.py # src/fastoad/models/performances/mission/segments/altitude_change.py # src/fastoad/models/performances/mission/segments/base.py # src/fastoad/models/performances/mission/segments/cruise.py # src/fastoad/models/performances/mission/segments/hold.py # src/fastoad/models/performances/mission/segments/start.py # src/fastoad/models/performances/mission/segments/taxi.py # src/fastoad/models/performances/mission/segments/tests/test_taxi.py # src/fastoad/models/performances/mission/segments/transition.py # src/fastoad/openmdao/variables/variable.py # tests/integration_tests/oad_process/test_oad_process.py
# Conflicts: # src/fastoad/models/performances/mission/openmdao/tests/data/test_mission.xml # src/fastoad/models/performances/mission/tests/test_routes.py
Codecov Report
@@ Coverage Diff @@
## master #473 +/- ##
==========================================
+ Coverage 83.42% 83.47% +0.05%
==========================================
Files 122 123 +1
Lines 5520 5490 -30
Branches 837 836 -1
==========================================
- Hits 4605 4583 -22
+ Misses 766 751 -15
- Partials 149 156 +7
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
There was a problem hiding this comment.
Thank you for this PR.
It's good to have a correct takeoff simulation functional.
Yet, we will need to do some rework. The main reason is that your work highlights some new need that we should address in a generic way. We will have to discuss about that to find the best way.
There are also some needed change in your code. I did not check all thoroughly, since I expect we will have to change things anyway. Yet you will find my remarks in the review details, and you may process them without waiting our discussion (but no hurry, of course).
src/fastoad/models/performances/mission/mission_definition/mission_builder/mission_builder.py
Outdated
Show resolved
Hide resolved
| # If ground_effect tag, call the variables needed for the ground effect model declared | ||
| if isinstance(polar_structure, dict): | ||
| keys = list(polar_structure.keys()) | ||
| if GROUND_EFFECT_TAG in keys: | ||
| gnd_effect = polar_structure[GROUND_EFFECT_TAG] | ||
| polar = Polar() | ||
| for name, value in polar.get_gnd_effect_model(gnd_effect).items(): | ||
| polar_structure[name] = {"value": value["name"], "unit": value["unit"]} | ||
|
|
There was a problem hiding this comment.
I understand the need, but I don't like to have such specific code there. We should try to find if a more generic mechanism can be proposed.
There was a problem hiding this comment.
I agree, although I tried to make it as generic as possible within the current mission builder process.
| "k_cd": {"name": "tuning:aerodynamics:aircraft:cruise:CD:k", "unit": None}, | ||
| } | ||
|
|
||
| def __init__(self, input_arg=None): |
There was a problem hiding this comment.
I have hard time imagining something less explicit than input_argas an argument name :)
The thing is that you expect either a 2-uple of arguments for CL and CD, or a dict with very specific keys (though optional) including CL and CD. Why not simply having arguments with a None default value?
There was a problem hiding this comment.
Fixed in 36bab9f , now the argument can only be to be a dictionary but it ought to be a generic input name as it can contain different variables depending on the ground effect and takeoff options
| input_arg[0], input_arg[1], kind="quadratic", fill_value="extrapolate" | ||
| ) | ||
| elif isinstance(input_arg, dict): | ||
| key = list(input_arg.keys()) |
There was a problem hiding this comment.
Useless and (slightly) harmful.
Harmful because you use a variable name that is not plural. Therefore the name does not tell what it actually is.
Useless because you just what to test if a key is in a dict, which can be done simply with if my_key in my_dict: .
There was a problem hiding this comment.
Fixed in 36bab9f
| self._span = input_arg["span"] | ||
| self._lg_height = input_arg["lg_height"] | ||
| self._induced_drag_coef = input_arg["induced_drag_coef"] | ||
| self._k_winglet = input_arg["k_winglet"] | ||
| self._k_cd = input_arg["k_cd"] |
There was a problem hiding this comment.
I understand why you did not want to have all these variables as inputs along already existing ones.
But the correct way to manage that is to create a class dedicated to ground effect. And an instance of this class should be expected in the __init__ of the polar.
This looks even more a good approach since you want to allow the choice between several ways of computing the ground effect.
There was a problem hiding this comment.
Implemented in 36bab9f
| - segment: ground_speed_change | ||
| target: | ||
| equivalent_airspeed: | ||
| value: data:mission:operational:takeoff:Vr | ||
| - segment: rotation | ||
| target: | ||
| delta_altitude: | ||
| value: 35 | ||
| unit: ft | ||
| - segment: end_of_takeoff | ||
| time_step: 0.05 | ||
| target: | ||
| delta_altitude: | ||
| value: 35 | ||
| unit: ft |
There was a problem hiding this comment.
I understand the need to implement 3 segments. But should we not propose only one, that would bundle the 3 parts, for the mission file? I know this is currently not possible, but if makes more sense, we could make it possible.
Also, why rotation and end_of_takeoff have the same target?
There was a problem hiding this comment.
About the last question, the rotation segment stops when the aircraft lifts off. At least this is the criterion to stop the segment since the reaction force from the ground disappears, it is necessary to change the equations and so change segment. I could not translate this criterion into a target, so it is set to 35ft arbitrarily without perturbing the takeoff simulation.
About the first remark, it would make more sense for the user indeed
-created abstract PolarModifier class -GroundEffect inherite from PolarModifier -instance of PolarModifier is expected for each timestep segment -default behaviour is to return legacy polar -polar is modified to store CL vs alpha interpolation (option) Still to do: -treat PolarModifier arguments passed by the mission file -mechanic to instantiate the correct PolarModifier from the mission file.
Polar modifier now returns a copy of polar Clean up
Need to enter delta altitude in takeoff with Ground Effect instead of absolute altitud
… of 1000m. Need to enforce AoA definition for Polar
# Conflicts: # pyproject.toml
…ground altitude in takeoff segments.
Hello,
This is a contribution to include take off simulation in the performance module. The newly created segments allow the simulation of ground acceleration and deceleration (for start and stop test), rotation and lift-off up to the safety altitude. The use of a polar with ground effect is included (although optional and not very significant for SMR). Tests files together with the specific mission definition file are provided and documentation is included.
What is left to decide after an initial review:
-Decide to include or not the take-off manoeuvre in the default mission file,
-Decide to provide or not a module to compute an adequate Vr as this speed must be chosen by the applicant (an input as of today, if it is too low, it stops the mission).
Thanks for your review