Skip to content
This repository has been archived by the owner on Aug 13, 2020. It is now read-only.

WIP: Classes refactor & scipy.ode dense output #104

Merged
merged 105 commits into from
Nov 19, 2017

Conversation

AlexS12
Copy link
Member

@AlexS12 AlexS12 commented Aug 27, 2017

It's time to come back!

This pull requests introduces three main changes:

  1. Classes refactor: some attributes and methods have been moved (specially affecting system and model). (@Juanlu001)
  2. EulerAngles system equations have been written as a single function (@DLpadilla), given that decoupling kinematic angular and linear equations from momentum equations led to instabilities Simulation stability #89 (jacobians need to be rewritten again).
  3. Integration dense output (https://stackoverflow.com/questions/12926393/using-adaptive-step-sizes-with-scipy-integrate-ode) is now used as simulation result.
  • As a side effect, inputs_generator has been rewritten in order to return not vectors but functions that can be evaluated at arbitrary times. @olrosales

Moreover, the documentation of more classes and methods has been added or completed (even if there are still some modifications and additions to be done)

I know this is a pretty large pull request with more than desirable changes to review at once (I apologize for that) and there are still some changes and fixes that I want to do before merging (updating the examples among them).

I have cited some of you to keep an eye on certain parts of the code and I encourage everyone (@AeroPython/pyfme) to review the state of the repository before and after this pull request so we can restart the activity with small incremental changes.

@astrojuanlu
Copy link
Member

Codecov didn't trigger and Travis already submitted the results... Looking into it.

@astrojuanlu
Copy link
Member

astrojuanlu commented Nov 12, 2017

The results are there https://codecov.io/gh/AeroPython/PyFME/pull/104/commits and both GitHub and TravisCI report all systems operational. @AlexS12 would you please do git commit --allow-empty -m "Trigger CI" && git push again? The Codecov thing might be an unrelated permissions issue, will try to fix it today.

@astrojuanlu
Copy link
Member

(Edit: commit && push)

@AlexS12
Copy link
Member Author

AlexS12 commented Nov 12, 2017

~/python/PROJECTS/PyFME $ git commit -m --allow-empty -m "Trigger CI"
On branch classes-refactor
nothing to commit, working directory clean

@codecov-io
Copy link

codecov-io commented Nov 12, 2017

Codecov Report

Merging #104 into master will increase coverage by 5.6%.
The diff coverage is 89.37%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #104     +/-   ##
=========================================
+ Coverage   87.25%   92.86%   +5.6%     
=========================================
  Files          28       41     +13     
  Lines        1914     2185    +271     
=========================================
+ Hits         1670     2029    +359     
+ Misses        244      156     -88
Impacted Files Coverage Δ
src/pyfme/utils/coordinates.py 100% <ø> (ø) ⬆️
src/pyfme/models/__init__.py 100% <ø> (ø) ⬆️
src/pyfme/utils/altimetry.py 100% <ø> (ø) ⬆️
src/pyfme/environment/tests/test_isa.py 100% <ø> (ø) ⬆️
src/pyfme/utils/anemometry.py 93.54% <0%> (ø) ⬆️
src/pyfme/tests/test_pyfme.py 100% <100%> (ø)
src/pyfme/aircrafts/cessna_172.py 100% <100%> (+13.51%) ⬆️
src/pyfme/environment/__init__.py 100% <100%> (ø)
src/pyfme/aircrafts/tests/test_cessna_172.py 42.85% <100%> (-57.15%) ⬇️
src/pyfme/models/euler_flat_earth.py 100% <100%> (ø) ⬆️
... and 46 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f2a8c2f...461ca9a. Read the comment docs.

Copy link
Member

@astrojuanlu astrojuanlu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I already had a look to this in full. Comparing the diff makes no sense since this is a full refactor of almost everything. The classes look good, the notebook is awesome and for me this is ready to go :)

@AlexS12
Copy link
Member Author

AlexS12 commented Nov 12, 2017

I made a las push just to make sure my changes trigger all the CI process correctly.

I will write a final message here covering what has been going on and what is still missing. After I will open some new Issues for different tasks. I hope we can recover a healthier and active workflow from now on. This long pull request in terms of time and amount of changes shouldn't be the norm 😇

@AlexS12
Copy link
Member Author

AlexS12 commented Nov 12, 2017

@AeroPython/pyfme
If you have just arrived here and think this is too long and dense, please have a look at this notebook (https://github.com/AlexS12/PyFME/blob/classes-refactor/examples/How%20it%20works.ipynb) and you can come back here later if you want!

This pull request started with the objective of changing the System and Model classes (those in charge of the integration process) so that the architecture of the simulator was simpler and easier to understand.

Moreover, the integration process had been reported to suffer from some problems (#89):

  • even if the solver was using adaptive step size, forcing terms in each set of equations were assumed to be constant for each time step.
  • as the system integrated first, linear and angular momentum conservation equations; secondly, the angular kinematic equations and finally, the linear kinematic eqs. The output from the previous was considered constant during the multiple adaptive steps of the next set of equations (ie. u, v, w, p, q, r were considered constant when integrating theta_dot, phi_dot, psi_dot) This included total forces and moments during momentum conservation equations.

So EulerAnglesFlatEarth system equations have been rewritten as a single function to be integrated by Scipy. The jacobians are still to be rewritten in case we want to use them for the integration.

In the meanwhile a new version of scipy came out, so the ode class has been dropped and the integration is carried out using scipy.integrate.solve_ivp (https://scipy.github.io/devdocs/generated/scipy.integrate.solve_ivp.html). This has simplified the workflow, but as the set_solout method has dissapeared, we still need to look for a new way to have a dense output result (using the dense output from solve_ivp is not enough because only contains the state vector, many other variables must be updated in that case). Currently the simulator returns outputs for the time steps the user configures at the beginning of the simulation.

Some classes have disappeared and many others have been created, a diagram is pending, but briefly:

  • The System class was intended to update variables not present in the dynamic system that could be useful for the simulation (ie. the model could be integrating in Earth coordinates: x_e, y_e, z_e; but the latitude and longitude could be used to check the wind magnitude and direction in the environment). This class has been disassembled in different objects for different groups of variables: posisiton, velocity, attitude, angular velocity, acceleration, angular acceleration... (the all live in pyfme/models/state/. The AircraftState has one position, one velocity... and is the input and output of every time step integration. All these classes rely on the pyfme/utils/coordinates.py functions, that are tested, but the update methods of the classes itself are not. This is the very next thing to do.

  • The trimmer is not a Simulation method anymore and has been move to a regular function that receives an aircraft, the environment, the initial controls and the necessary information about the initial state (position, psi, TAS, turn_rate and gamma) It returns a trimmed_state and the trimmed_controls.

  • The Aircraft class has suffered minor modifications. It has to be broken up into smaller and simpler pieces. @sanguinariojoe (https://github.com/sanguinariojoe/PyFME) made some cahnges here that should be taken into account as they are useful and add potential new functionalities (such as moving center of gravity). @olrosales, @aqreed I would like to keep your aircrafts updated when this happens, let me know if you are still interested in contributing.

  • Documentation has been extended and improved. I appreciate any contribution here, from PR to messages or discussions addressing not documented or hard to understand parts.

  • There is only one example notebook now which tries to explain the necessary steps to run a simulation. I have removed the rest of them because because they consisted in different maneuvers only (the simulation process was the same except for the controls). I have no problem in including them again if someone wants to make an update, but I think it is much more interesting to complete this one first.


I will wait one week from now (19th November) for reviews and suggestions, but I understand that under such a big amount of changes at the same time, this is a difficult task. Anyway any kind of contribution is appreciated and welcomed. 😃

@AlexS12
Copy link
Member Author

AlexS12 commented Nov 18, 2017

Last commits are an answer to: #95
Last day for comments before merging!

@aqreed
Copy link
Member

aqreed commented Nov 18, 2017

@AlexS12 Farewell my dear .py examples👋

@AlexS12
Copy link
Member Author

AlexS12 commented Nov 18, 2017

@aqreed That was a great contribution (as well as the aircraft 🛩️). New examples that make the difference again are more than welcome! 👌

@AlexS12 AlexS12 merged commit 7be3c2d into AeroPython:master Nov 19, 2017
@astrojuanlu
Copy link
Member

ALELUYA

This was referenced Nov 19, 2017
@AlexS12 AlexS12 deleted the classes-refactor branch November 19, 2017 22:00
Copy link
Contributor

@gurkanctn gurkanctn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just sum the winds from the Environment module? instead of the subtraction. The environment should give wind-speed vector in the Body frame, and therefore summing would look more natural?.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants